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

DM-23173: Change W504 to W503 and fix flake8 warnings #156

Merged
merged 1 commit into from Apr 26, 2020

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Apr 25, 2020

No description provided.

@r-owen r-owen requested a review from timj April 25, 2020 23:10
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one minor comment.

"measAlg.PsfCandidateF or other type with a getSource() method: %s" % (e))
raise RuntimeError(f"Candidate List is of type: {type(candidateList[0])} "
"Can only make candidate list from list of afwTable.SourceRecords, "
"measAlg.PsfCandidateF or other type with a getSource() method") from e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People probably still might like the exception string in the new exception string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the point of "raise...from" was to avoid the need for that. But I will put it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the from e is not actually useful here (it will be obvious what is going on) so I'll restore the original -- the RuntimeError message will contain the error and no from e.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the from e or add from None, if it's not too late - without one of those, exceptions raised inside except blocks get a confusing "while processing this exception, another one was raised" (or something like that) message.

(I'm sure the original code was written before that behavior existed, but it's definitely something we should be fixed in re-raises as we encounter them.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is too late. But in any case I strongly feel that the from is not useful here. This is just a way to test if getSource() works and complain that the data is the wrong type if not. That is all the user really needs to know. I especially think it would be wasteful to quote the original exception and also raise from it.

I suppose one could call hasattr instead to save the first exception, though that would not catch the case that hasattr exists but cannot be called as a function with no arguments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that it's too late, but for future scenarios, this is exactly what from None is for.

@r-owen r-owen force-pushed the tickets/DM-23173-ip_diffim branch from 8f88671 to 4b110f5 Compare April 26, 2020 02:55
@r-owen r-owen merged commit 59f0e8a into master Apr 26, 2020
@r-owen r-owen deleted the tickets/DM-23173-ip_diffim branch April 26, 2020 04:56
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