tbird20d / grabserial

Grabserial - python-based serial dump and timing program - good for embedded Linux development
GNU General Public License v2.0
198 stars 79 forks source link

Fixes output filename expansion and adds clarity to documentation #44

Closed AlphaSierraHotel closed 3 years ago

AlphaSierraHotel commented 4 years ago

with thanks to @Jeroen6 for his input

AlphaSierraHotel commented 4 years ago

From wikipedia - iso8601

"... is an international standard covering the exchange of date- and time-related data."

As this is strictly being used to create a filename when specifying a shortcut (-o "%") and not necessarily going to be used to "exchange" the timestamp of the file with other systems, I propose that a non-standard format would be acceptable.

The replace() function didn't work as intended and is actually what caused the strftime() expansion to fail.

I can add the system-specific logic back in so that -o "%" expands differently on Windows than it does on other systems if you want to allow for the colons on non-Windows systems. I was endeavouring to keep it OS agnostic. Your choice though, so let me know.

Edit: grammar, formatting and emphasis

Jeroen6 commented 4 years ago

I understand. The : looks nicer and is better tk spec, but then there are also windows users.... Plus, instead of “-“ one could also argue “.” Or “_”.

If you feel bad for choosing or want to avoid the above, you can also remove the default feature entirely and merely suggest to use "%Y-%m-%dT%H-%M-%S" or "%Y-%m-%dT%H:%M:%S" in the docs.

In fairness, the “%” feature is limited without support for paths and extensions. However easier to do on a shell by hand.

AlphaSierraHotel commented 4 years ago

It may go without saying, but this only impacts the output file name and has no impact on the format of times reported within the data. Within the data (ie. with -T ) the format remains standard with colons. (systime_format = "%H:%M:%S.%f")

tbird20d commented 4 years ago

OK - let's just go with the dashes. People (like myself), can use colons in fully-specified format strings if they like. It's not that hard. I can get what I want with "%F_%T", which is simple enough.

I'm not sure we need the check for "%" before formatting with strftime, in the command line argument processing section. We don't use that conditional anywhere else we create a new out_filename and use strftime with out_pattern.

AlphaSierraHotel commented 4 years ago

We do need it but only that one time at invocation. It effectively changes the passed in argument, expanding it to the full timestamp pattern and we do it before assigning it to the out_pattern variable. It's the out_pattern that is used to re-create a new out_filename value whenever it's appropriate.

AlphaSierraHotel commented 4 years ago

Would you prefer the "T" between the date and time? It's a dash currently in this Pull request.

tbird20d commented 4 years ago

As long as we're dropping support for compatibility with iso8601 format, how about underscore? I think that's more readable than either the T or dash. I'm open to other opinions though.

AlphaSierraHotel commented 4 years ago

I agree. That ought to do it then.

AlphaSierraHotel commented 4 years ago

I'm not sure we need the check for "%" before formatting with strftime, in the command line argument processing section. We don't use that conditional anywhere else we create a new out_filename and use strftime with out_pattern.

Ugh, I just re-read this and perhaps I interpreted it incorrectly the first time. The "%" shortcut isn't strictly required and does have limited use cases. However, it is certainly handy, mainly in quickly capturing output to file from a command line in a diagnostic scenario.

We're beginning to over-think this though. I would recommend keeping it and perhaps consider updating or refining documentation in the near future instead.

github-actions[bot] commented 3 years ago

Stale pull request message

AlphaSierraHotel commented 3 years ago

As far as I know, this is ready to be merged.

tbird20d commented 3 years ago

I refactored these a bit, but this change is now in the master branch.

Thanks. -- Tim