Fix code style issues flagged by LGTM#1559
Conversation
|
@chrisorner thanks for fixing these issues! There's two minor issues flagged by stickler (lines that are too long) I suppose an entry into the appropriate whatsnew file would be appreciated - you can go ahead and add your name to the list of contributors too :) |
kandersolar
left a comment
kandersolar
left a comment
There was a problem hiding this comment.
Thanks @chrisorner! I'll trigger the tests to make sure everything still works, but here are a couple comments in the meantime.
| try: | ||
| data, meta = parse_epw(csvdata, coerce_year) | ||
| except Exception as e: | ||
| print(e) |
There was a problem hiding this comment.
I think we definitely want to propagate any parsing errors instead of printing them and letting execution continue, so better to get rid of the try/excepts in these two blocks.
There was a problem hiding this comment.
Thanks @chrisorner for the contribution and closing a long-standing annoying issue, and also thanks @kanderso-nrel for quick and thoughtful review. I apologize for commenting after the fact. I agree with Kevin (altho he might not agree with me). In my opinion we should restrict using print() in pvlib, except perhaps in examples, b/c it blocks the main execution thread which reduces performance. An alternative is logging which has many useful features, executes in separate thread, and therefore, does not block the main Python thread. However, as a rule pvlib has avoided logging except during development. Hope this makes sense to folks. Just my thoughts/opinions, definitely do not speak for anyone other than myself.
There was a problem hiding this comment.
@mikofski just to be sure, would you change anything about the state of this PR as it was merged? Note that this printing was removed in a later commit and didn't make it into the final squashed diff.
I definitely agree that print() inside pvlib itself is never (or at least very rarely) the best way to present information to the user.
There was a problem hiding this comment.
Nope, no changes. I was very happy with this PR and the review. Thanks again! Just wanted to make sure it was in the record that I extremely discourage print(), and I consider logging a better alternative although to date, hasn't been used in pvlib much.
kandersolar
left a comment
kandersolar
left a comment
There was a problem hiding this comment.
Codecov failure can be ignored. Thanks again @chrisorner
5 participants
remote-data) and Milestone are assigned to the Pull Request and linked Issue.