necouchman commented on PR #990:
URL: https://github.com/apache/guacamole-client/pull/990#issuecomment-2143192572
@kalaxcortez Thank you for contributing, here. Before we can accept this
contribution, a number of things need to be done:
* You need to request a Jira account and open a Jira issue, and your pull
request and commit messages need to be tagged with the Jira issue. You can look
at any of the existing closed, and many of the open pull requests for examples
of this.
* You need to follow established style within the code. For example, you're
cuddling braces (`} catch () {`) - they should be on new lines. Please adjust
your style to conform to the rest of the code.
* I think the `IOException` handling would actually be better placed in the
`FileGuacamoleProperty` class (in `guacamole-ext`), where it could easily be
used by any and all users of the class, rather than just these two properties.
It does not seem to be a situation unique to these particular usages of that
property.
* You're catching both `IOException` and `GuacamoleException`, here, and
printing the stack trace - this defeats the purpose of having this particular
function throw the `GuacamoleException`. If you're going to catch
`IOException`, it should be re-thrown as a `GuacamoleException`, and the
throwing of `GuacamoleException` should be left alone.
> please validate and if you do not like the structure, change it but solve
it
I believe your suggested changes have merit, and, with some tweaks, would be
quite valuable to have added to the project. I'd highly encourage you to take
the time to do the steps mentioned above and clean this up for inclusion - and,
if you're so inclined, find other ways to contribute to the project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
