some new features because south america#46
some new features because south america#46bhbraswell wants to merge 1 commit intoApplied-GeoSolutions:devfrom
Conversation
ra-tolson
left a comment
ra-tolson
left a comment
There was a problem hiding this comment.
I don't really understand mt so I feel weirdly unqualified to do code reviews. I mean, yeah, I ported the thing to python 3, but mostly that was an automated process wherein I ran some tests and didn't get into the internals much.
So this is more an advisory review, things I'd change even though I recognize my position of relative ignorance.
| step['nout'] = step['nin'] | ||
| try: | ||
| step['nyrout'] = int(mod.get_nyrout(step['nyrin'], step['params'])) | ||
| except: |
There was a problem hiding this comment.
Better to catch a specific exception type for safety.
Optional: Also, exceptions that pass silently are pretty rare; consider emitting a message to the user even if you don't re-raise.
| else: | ||
| step['nin'] = thisnin | ||
| if 'nyrin' in step: | ||
| assert step['nyrin'] == thisnyrin, "Number of years do not match" |
There was a problem hiding this comment.
What would happen if the user disabled assertions when running this? That's how to tell if it should be a raise ValueError instead.
| # secret way to test | ||
| results = [func(jobs[-nproc-1])] | ||
| else: | ||
| raise Exception("nproc can't be zero") |
There was a problem hiding this comment.
This isn't Exception, this is ValueError. Never raise Exception; either pick a subtype or make a new one.
|
|
||
|
|
||
| @pytest.mark.parametrize("nproc", [1, 2]) | ||
| def test_passthrough2(nproc, tmpdir): |
There was a problem hiding this comment.
Copypasta is bad. Extend the parametrization scheme instead so the existing test covers more cases.
Also, if you do, then test_passthrough2 is going away as a name, but know that it's a much better idea to name your tests verbosely so people can see what went wrong in test reports. Long, sloppy names aren't so bad for tests, eg, test_passthrough_this_time_with_weird_regex.
There was a problem hiding this comment.
PS you can instead move some common code into a fixture and call it from both, if the two tests are different enough to warrant it.
2 participants
shifttime.pyx module in case the season starts at a weird time
trimyr.pyx module because shifttime can result in a trailing useless year (also could consider an option to shifttime, but this seemed potentially useful
Modules can now change the number of years they output (nyrout), e.g. trimyr.pyx
Regex can now contain a group besides the date, but if so, they need to be labeled (backwards compatible), in our use case that group is used to handle data from different locations mixed in the same directory