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

JvTFDays: Map column under certain conditions not found #127

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MHumm
Copy link
Contributor

@MHumm MHumm commented May 20, 2019

Handle exceptions so it doesn't crash. Fix for acknowledged Mantis issue 6409.
http://issuetracker.delphi-jedi.org/view.php?id=6409

@MHumm
Copy link
Contributor Author

MHumm commented May 20, 2019

Ok, a further look at the source and the notes added to the issue reveal, that the other note has already been implemented while the first suggestion with catching the exception (which is the contents of my pull request) has not. I also saw that the routine where the note wanted changes has been commented out and the version from the note including commenting code out and commenting that the modified code is some untested source only are still there. What to do with that?

@obones
Copy link
Member

obones commented May 22, 2019

I despise empty try..except blocks, they just hide things and they come to haunt you later on.
The real source of the issue should be found and addresses

@MHumm
Copy link
Contributor Author

MHumm commented May 23, 2019

Removed the try except, as the called method has received some fix which is more in direction to fix the real problem.

@obones
Copy link
Member

obones commented May 24, 2019

So, if I read the final content correctly, there is nothing left to change?

@MHumm
Copy link
Contributor Author

MHumm commented May 24, 2019

Applied the fix suggested in the note to the Mantis issue, which catches exceptions and sets values to sensible values.

@obones
Copy link
Member

obones commented May 28, 2019

I'm still not happy with try..except that hide all exceptions. If an error occurs because of anything but the original issue, it gets masked and will harm us later on.

@ronaldhoek
Copy link
Contributor

I'm still not happy with try..except that hide all exceptions. If an error occurs because of anything but the original issue, it gets masked and will harm us later on.

I agree with @obones - the BUG needs to be reproducible, so the actual source of the exception can be found and handled/dealt with.

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.

3 participants