-
Notifications
You must be signed in to change notification settings - Fork 94
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
Refactor the area loading internal function #451
Conversation
…_yaml` utility functions
How is this different from |
it probably isn't, I wasn't aware it existed :) I'll have a look |
Sorry about that @gerritholl , I was so sure this didn't exist already. Anyway, I removed the meat of this and just left the small refactoring. |
#!/usr/bin/env python | ||
# -*- coding: utf-8 -*- | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these lines actually do anything useful for a test module? As far as I know, the first line only matters for commandline scripts and the second is redundant in Python 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought about that tbh. For consistency, maybe we should have a PR just for removing all of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I could do it here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably remove them. I've only ever added them for consistency. As a library, we could probably remove the usage of both in all pytroll packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just one minor question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Regarding the test failures: This has to do with new versions of setuptools. I would think we should be fine, but obviously things aren't working. It looks like there has been a patch release of setuptools since the last CI run here. It may be worth restarting them @mraspaud but I wanted to see what you thought. I can't tell from the output what version of setuptools was used in the test install. Or I wonder if this requires a new version of versioneer. |
Ah, there was a versioneer release to fix it: https://github.com/python-versioneer/python-versioneer/releases/tag/0.23 |
Updated versioneer, let's see how it goes |
Codecov Report
@@ Coverage Diff @@
## main #451 +/- ##
=======================================
Coverage 94.28% 94.28%
=======================================
Files 68 69 +1
Lines 12361 12369 +8
=======================================
+ Hits 11654 11662 +8
Misses 707 707
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This PR adds the possibility to create an area definition from a yaml string. Socreate_area_def_from_dict
andcreate_area_def_from_yaml
utility functions were added.This PR refactors the area loading a little.