Ben1152000 / sootty

A command-line tool for displaying vcd waveforms.
BSD 3-Clause "New" or "Revised" License
44 stars 12 forks source link

Change filename to be a positional argument #53

Closed Ben1152000 closed 1 year ago

Ben1152000 commented 1 year ago

This PR contains the following changes:

KarthikL1729 commented 1 year ago

Looks good to me, fixed a small bug in the reloading part, functionality wise it's perfect. Also really like the new dict representation in the save file!

However I do think the old date format would be preferred for most usecases?

I also think providing absolute path would be better as that would allow us to reload from anywhere in the directory tree, will push a fix for that.

KarthikL1729 commented 1 year ago

Pushed the fix and tested functionality, would like your opinion on the date format!

Ben1152000 commented 1 year ago

However I do think the old date format would be preferred for most usecases?

I went with the unix timestamp since it's very easy to compare two values. I'm not sure if there are any weird issues when comparing dates, but if you think it's better to have readability feel free to change it.

I also think providing absolute path would be better as that would allow us to reload from anywhere in the directory tree, will push a fix for that.

Agreed, this behavior seems more reasonable.

KarthikL1729 commented 1 year ago

However I do think the old date format would be preferred for most usecases?

I went with the unix timestamp since it's very easy to compare two values. I'm not sure if there are any weird issues when comparing dates, but if you think it's better to have readability feel free to change it.

I also think providing absolute path would be better as that would allow us to reload from anywhere in the directory tree, will push a fix for that.

Agreed, this behavior seems more reasonable.

I think the unix timestamp is fine as well, not an issue per se. Will solve conflicts and approve.