Bugfix Downloader handling of trailing empty lines#223
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 90.46% 90.48% +0.02%
==========================================
Files 19 19
Lines 3040 3048 +8
==========================================
+ Hits 2750 2758 +8
Misses 290 290
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:
|
pyorbital/tlefile.py
Outdated
|
|
||
| def _decode_lines_with_platform_header(fid): | ||
| tle = "" | ||
| try: |
There was a problem hiding this comment.
you could use suppress(StopIteration) here.
Sorry is this is a stupid question however:
Where does the StopIteration come from? As I understand it, that error comes if we iterate too much with next, but we want to stop at the second line when there are three, so why the error?
There was a problem hiding this comment.
The error comes when we start parsing the last, empty, line of
1 ... first TLE line
2 ... second tle line
and then try to get the next line.
There was a problem hiding this comment.
Sorry for my ignorance on this subject, but isn't that an error case then? If we expect a third line and there isn't one?
Otherwise, all this code kind of feels like another generator abstraction is needed. Like:
def decoded_line_pairs_from_file(fid, platform):
l0 = l1 = None
for line in fid:
line = _decode(line) # also do the .strip() here maybe?
if l0 is None:
l0 = line
continue
if l1 is None:
l1 = line
if l0 != platform:
yield l0, l1
l0 = l1 = None
continue
if l0.startswith(deisgnator):
yield l1, l2
l0 = l1 = None
... something? ...
I don't know. Just a gut reaction to see the StopIteration stuff.
There was a problem hiding this comment.
There are two valid cases for the TLE:
NOAA-20
1 ...
2 ...
and the same without the NOAA-20 line. So either 3 or 2 lines, out of which only the last two are required.
I see that the original code is bad(tm), but didn't want to go that far to rewrite it, just to stop the files with trailing empty lines crashing the code.
There was a problem hiding this comment.
Ok, thanks. I still don't see the failure case though. Maybe I need to try to reproduce this to understand.
There was a problem hiding this comment.
If I restore the version from this PR and add a print to the except StopIteration block and then run the tests with pytest -s pyorbital/tests/test_tlefile.py I don't see the print in the output.
There was a problem hiding this comment.
WTF 😂 This time I even wrote the tests first... And this is how it now fails in production with files having trailing empty lines. Locally I tested with Python 3.13.
I'm travelling tomorrow, so can have another look on thursday.
There was a problem hiding this comment.
Sanity check:
(polar2grid_py312) davidh@dart:~/repos/git/pyorbital$ git branch
* bugfix-downloader-skip-empty-lines
...
(polar2grid_py312) davidh@dart:~/repos/git/pyorbital$ git diff --stat main
pyorbital/tests/test_tlefile.py | 15 +++++++++++----
pyorbital/tlefile.py | 34 ++++++++++++++++++++++++++++------
2 files changed, 39 insertions(+), 10 deletions(-)
(polar2grid_py312) davidh@dart:~/repos/git/pyorbital$ git diff
diff --git a/pyorbital/tlefile.py b/pyorbital/tlefile.py
index 33f7bbf..2c9fd50 100644
--- a/pyorbital/tlefile.py
+++ b/pyorbital/tlefile.py
@@ -433,6 +433,7 @@ def _decode_lines_with_platform_header(fid):
tle = _merge_tle_from_two_lines(l_1, l_2)
except StopIteration:
# There were empty lines at the end of the file
+ print("Stop iter!!!")
pass
return tle
@@ -448,6 +449,7 @@ def _decode_lines_without_platform_header(fid, l_0, platform, only_first, open_i
LOGGER.debug("Found platform %s, ID: %s", platform, SATELLITES[platform])
except StopIteration:
# There were empty lines at the end of the file
+ print("Stop iter!!!")
pass
return tle
(polar2grid_py312) davidh@dart:~/repos/git/pyorbital$ pytest -s pyorbital/tests/test_tlefile.py::TestDownloader::test_read_tle_files
=============================================================================================== test session starts ================================================================================================
platform linux -- Python 3.12.4, pytest-8.3.2, pluggy-1.5.0
rootdir: /home/davidh/repos/git/pyorbital
configfile: pyproject.toml
plugins: lazy-fixtures-1.1.1
collected 4 items
pyorbital/tests/test_tlefile.py ....
================================================================================================ 4 passed in 0.16s =================================================================================================
There was a problem hiding this comment.
Oh, the actual failure I've been having is when there are two or more empty lines. Test case added.
There was a problem hiding this comment.
I added a check for the empty platform string, so the try/except are not needed. They weren't needed in the other branch anyways..
djhoese
left a comment
djhoese
left a comment
There was a problem hiding this comment.
Nice job. Glad it was easier to clean up when the issue was better understood. I feel like the if statements could be rearranged to make more sense but that doesn't have to happen here or now.
Looking at the logic is seems very much like some platform checks and some "which lines do we use" rearranging. There are also log messages and if statements with no else block which seem like there could be some short cuts (early return or something similar). But this PR as-is is pretty obvious how you got from one thing to the other so 👍.
5 participants
I noticed that if there are any empty lines after the TLEs in files stored locally and used in forming the final TLE set, the parsing fails. This PR adds error handling for the empty lines.