-
Notifications
You must be signed in to change notification settings - Fork 79
Add fallback when TLES environment variable not listing any files
#205
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
+ Coverage 89.58% 89.78% +0.19%
==========================================
Files 17 17
Lines 2784 2809 +25
==========================================
+ Hits 2494 2522 +28
+ Misses 290 287 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| LOGGER.debug("Fetch TLE from the internet.") | ||
| uris = TLE_URLS | ||
| open_func = urlopen | ||
| LOGGER.warning("TLES environment variable points to no TLE files") | ||
| uris, open_func = _get_internet_uris_and_open_method() |
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 don't understand this change. What does this do? Reduce the number of lines by 1? Oh it adds a warning along with the debug log message?
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.
First the code needed to be refactored so that the "get TLEs from internet" case were a function. Then I added the warning when TLES env variable is given and didn't provide any files and the fallback of getting the data from internet is used.
|
Thanks for getting this implemented so quickly. My biggest worry with this, not fully understanding how users use this, is that someone (me) may want to turn this off, or may want to do the download manually, or may want to run this on a system that can't access the internet so this would fail anyway. Oh or they may be upset that a ton of files got downloaded when they were just testing things. Pyspectral does something similar within its configuration file. |
Took only 4.5 years 😅
The other option would be to fail immediately if There are only 9 text files that are downloaded. And as far as I can tell, the previously downloaded data are overwritten, so not much are accumulated. |
djhoese
left a comment
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.
This looks good enough to me from the little experience I have with it. I assume if someone is customizing the "TLES" environment variable then they know that things might be downloaded. Should this be documented somewhere more public?
Oh one downside to this would be someone having a typo in their TLES glob pattern and then they get multiple copies but maybe that's OK.
|
The idea of |
mraspaud
left a comment
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, just one comment.
mraspaud
left a comment
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
Fixes #74 with a fallback to downloading TLEs from internet.