Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saved original filename is based on activity ID #66

Closed
wants to merge 0 commits into from

Conversation

cristian5th
Copy link
Contributor

It is risky to create the original filename based on the contents of the zip file. Actually, Garmin has recently changed this filename.

The proposal is to make a totally new filename based on the activity ID, independently of how Garmin has decided to name the downloaded file.

Solves issue #63

@cristian5th
Copy link
Contributor Author

Second commit is to save gcexport.log file into the export directory so all files are located at the same place

Copy link
Contributor

@bxsx bxsx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for the PR. Technically, I am concerned that this PR is not complete or breaks the common protocol.
Activity files other than original have a different file name template. I would expect it to be consistent.

I will leave another comment in #63 on the motivation of this feature request.

Second commit is to save gcexport.log file into the export directory so all files are located at the same place

Nice improvement. In case this PR would be rejected, I would still vote for cherry-picking this commit.

@bxsx
Copy link
Contributor

bxsx commented May 29, 2022

Oh, I misread the PR (edited previous comment)!

In any case, if we wanted to take this approach, it would be nice to handle multiple files in the archive as well (see #63).

@pe-st
Copy link
Owner

pe-st commented May 31, 2022

Second commit is to save gcexport.log file into the export directory so all files are located at the same place

Nice improvement. In case this PR would be rejected, I would still vote for cherry-picking this commit.

I'm not sure this is an improvement. If I run the script several times in a row I'd get several log files; I prefer to have only one log file to easily find the logs of all different script executions. But maybe this could be made configurable?

@cristian5th
Copy link
Contributor Author

How can this be if the filename and folder is the same every execution? What are the names of the different log files that you get with every execution?

@pe-st
Copy link
Owner

pe-st commented May 31, 2022

How can this be if the filename and folder is the same every execution? What are the names of the different log files that you get with every execution?

That really depends on your workflow. The default export directory is ./YYYY-MM-DD_garmin_connect_export, i.e. changing every day. And for my tests I often build the the export directory with the suffix `date +%Y-%m-%d_%H-%M-%S`, i.e. a separate directory for every run. So if the logfile is written to the export directory, I get a separate logfile for every execution.

But I agree that for some workflows it might be desirable to have the logfile in the export directory, so I think we should find a suitable command line option. Maybe a flag would be enough to decide between the script and the export directory? Or should we have more options to also allow an OS-dependent place (something like /var/log/...?), but that would need some research on what are the expected places per OS...

@bxsx
Copy link
Contributor

bxsx commented May 31, 2022

I can open a new PR so that a new argument --logfile indicates the requested location by the user. The default location would be derived from the --directory. This way you can always add --logfile=. to achieve the current behaviour.
Note: this is a breaking change but I think it's okay.

Are you OK with this solution?

@bxsx bxsx mentioned this pull request May 31, 2022
@pe-st
Copy link
Owner

pe-st commented Jun 7, 2022

I can open a new PR so that a new argument --logfile indicates the requested location by the user. The default location would be derived from the --directory. This way you can always add --logfile=. to achieve the current behaviour. Note: this is a breaking change but I think it's okay.

Are you OK with this solution?

Yes, thanks!

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.

3 participants