Skip to content

Conversation

@upsonp
Copy link
Contributor

@upsonp upsonp commented Dec 6, 2023

I've added a BTL file containing blank header lines and a unit test explaining a ValueError is thrown when there is a blank line in a BTL files header.

The reason is that the _parse_seabird() method assumes it's done reading if a line doesn't start with a '#' or '*' character. True if the line contains other strings like the column names, but not true if it's just a blank line in the header section.

If it is just a blank line, the _parse_seabird() function doesn't set the metadata['names'] correctly, then back in the read_btl() function when the function checks the metadata['names'].index('Date'), there is no 'Date' element, which causes the ValueError.

@upsonp
Copy link
Contributor Author

upsonp commented Dec 6, 2023

I'm sorry, I tried. This is a stupidly simple thing, but I can't figure out what the issue is without seeing what pre-commit.ci is checking.

@ocefpaf
Copy link
Member

ocefpaf commented Dec 7, 2023

I'm sorry, I tried. This is a stupidly simple thing, but I can't figure out what the issue is without seeing what pre-commit.ci is checking.

You can ignore the pre-commit.ci for now. We can fix that later. I'll check you PR ASAP. Thanks for submitting it!

upsonp and others added 2 commits December 9, 2023 12:41
Yes, looks good if that's how you want to tackle it

Co-authored-by: Filipe <ocefpaf@gmail.com>
@upsonp
Copy link
Contributor Author

upsonp commented Dec 11, 2023

I should have read that last update more closely before accepting it.

It's ok to skip the check to see if the '*' or '#' characters are at the beginning of the line if the line is blank, but the character check is still necessary along with the logic on how to handle it, which was being skipped because of where 'continue' was located. 😄

@upsonp
Copy link
Contributor Author

upsonp commented Dec 18, 2023

Just wondering if there's any update on this PR.

@ocefpaf
Copy link
Member

ocefpaf commented Jan 8, 2024

pre-commit.ci autofix

@ocefpaf ocefpaf merged commit 5f75b62 into pyoceans:main Jan 8, 2024
@ocefpaf
Copy link
Member

ocefpaf commented Jan 8, 2024

Just wondering if there's any update on this PR.

End of the year is always a bit slow. Thanks for your patience. I'll try to tag a new release soon.

@upsonp
Copy link
Contributor Author

upsonp commented Jan 8, 2024

Thanks a bunch ocefpaf!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants