whimwich
commented
2 years ago I am sorry it took me so long to get back to this. I have mostly minor comments.
There are lots of things to like, including: reading POs from a file, redirecting to a different file, the historical sub-directory, using argparse, dates in the data filenames, better format for the GPL header, making station codes case insensitive, etc. Spot checks show that I get the same results. The -f option still functions. Good work.
The most significant issue is that -a (--adjust) doesn't seem to do anything different than "normal" processing, but maybe this whole area needs to be reworked. You might say include -a to print adjusted offsets. Maybe that line should fit in an 80 column display.
I think it would be nice to have a -b option (or something) to automatically pick the BB POs instead of having to specify the file explicitly.
It looks like a -- is needed after the -x list if it is the last parameter, maybe there should be [--] before clocks (or [-x EXCLUDE [EXCLUDE ...] [--]]) in the example usage line.
Entering adjust t-h gives a strange error message, presumably because argparser failed. I wonder if errors like that can be caught and given a better message.
I notice if a station appears twice, it gets a Can't decode fmout error (twice). Could it be more specific? Handling clock breaks is one reason I suggested annotating the station code for breaks for the new report format. Otherwise maybe we have to move away from pandas for reading the input file. (That maybe necessary anyway if the new clock report format changes too much.) The previous version had no solution for this, you just got the last line for each station.
We lost the description of the output from the --help. Maybe that was just redundant.
It looks like we no longer have the description variable of the .dat files set in the files. That seems fine. It was a little awkward from a maintenance point-of-view. Maybe we could include the author (bec) in a comment (or in the filename) and the other comments. Having comments in the data files is good.
We seem to have lost the -v version number. Is that sort of thing is passé?
The self documenting of the PO filename is nice, but we don't get that for the "default" file; there is no date. I can't say I see a really obvious solution. There could just be a link to the default, which would, like any historical file, include the date in the filename. The link could be in top-level directory then (for sd too). That would be "self documenting". But then why not just have all the data in the data/ and skip historical/.
Contains the changes in my previous (closed) pull request: 'adjust' now reads peculiar offset files, added peculiar offsets from late last year, moved old peculiar offsets to 'data/historical/', and trimmed requirements.txt