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

Add Cross Platform Support for toggl-cli Path #295

Closed
wants to merge 3 commits into from
Closed

Add Cross Platform Support for toggl-cli Path #295

wants to merge 3 commits into from

Conversation

dkvc
Copy link
Contributor

@dkvc dkvc commented Feb 13, 2023

Move DEFAULT_CONFIG_PATH to toggl/utils/config.py
Use absolute import in commands.py
Closes #126

AuHau and others added 3 commits February 12, 2023 22:05
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Move DEFAULT_CONFIG_PATH to toggl/utils/config.py
Use absolute import in commands.py
Closes #126
@AuHau
Copy link
Owner

AuHau commented Feb 14, 2023

Thanks for the PR! It has been long standing issue :-)

I have a question though, how does this fits into multi-OS environment? I am not sure if toggl-cli really worked on Windows at some point, so that is not the platform I am concerned, but for example what about macOS? I don't think it follows XDG Base spec... So there should be some logic included that fallbacks to usage of ~ or similar if XDG_CONFIG_HOME is not defined...

@AuHau AuHau force-pushed the master branch 2 times, most recently from 44a18cd to c0cd4db Compare February 14, 2023 15:26
@dkvc
Copy link
Contributor Author

dkvc commented Feb 15, 2023

You are correct! It seems I failed to notice cross platform compatibility while looking through this. In such cases, pathlib.Path.expanduser("~/.togglrc"). This solution works in case of Python 3.5+.

If backward compatibility is required, then os.path.expanduser("~/.togglrc"). These solutions work throughout all platforms.

Does toggl-cli still supports any python version before 3.5?
Edit: I think it doesn't as any version before Python 3.6 is EOL. As far as difference between those two, the first one requires a Path object and returns as Path object, unlike the other. So, the first one might require more code, but it could be better long term, due to its object oriented file system paths.

@dkvc dkvc changed the title Follow XDG Base directory spec for DEFAULT_CONFIG_PATH Add Cross Platform Support for toggl-cli Path Feb 15, 2023
@AuHau
Copy link
Owner

AuHau commented Feb 15, 2023

We are supporting only currently supported Python versions, so that means >=3.7 (3.7 soon will be gone as well), so both options are fine ;-) I will leave that up to you choose.

The only thing I would ask you is, can you please either rebase or recreate the PR? I have been doing some bad stuff (eq. force pushing master 🙈 😅 ) in order to get the new CI release mechanism working... Which finally does, but it bit corrupted your PR unfortunately, sorry about that.

@dkvc
Copy link
Contributor Author

dkvc commented Feb 15, 2023

Sure, no worries 👍

@dkvc dkvc closed this Feb 15, 2023
@dkvc
Copy link
Contributor Author

dkvc commented Feb 15, 2023

I don't think DEFAULT_CONFIG_PATH in commands.py is doing anything. It can be a oversight. Can you take a look over it?

@AuHau
Copy link
Owner

AuHau commented Feb 15, 2023

You are right :-) It is the one in config.py that is used... Good catch!

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.

Follow XDG Base Directory Spec
2 participants