Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a fix for LDEV-2412 #6

Closed
wants to merge 1 commit into from
Closed

Conversation

cfmitrah
Copy link
Member

No description provided.

@michaeloffner
Copy link
Contributor

we cannot simply add an exception to that method, the method has as input a default value, that means in case it can not do the job the default value is returned and never an exception is thrown.

@bdw429s
Copy link

bdw429s commented Sep 6, 2019

The way this pull was implemented was wrong since it ignored the default value entirely, but I still think there should be no issue throwing the exception if the default value is null (not supplied). Silent errors that don't throw an exception but simply work incorrectly are the bane of developers. As an alternative, can @cfmitrah modify every place that calls this method to pass a default value like "not found" and then if "not found" comes back, throw an exception fromm the calling code? That is more work and a worse solution in my opinion than simply throwing the exception directly, but it will work.

@cfmitrah
Copy link
Member Author

cfmitrah commented Sep 9, 2019

OK, Thanks for your info. Will do the things whatever you mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants