feat: respect XDG spec for configuration files

[dkvc]

Use Path.expanduser from pathlib module instead of os module
Remove redundant DEFAULT_CONFIG_PATH in commands.py
Closes #126

[AuHau]

Thanks for the new PR! I'm bit confused thought. Shouldn't this PR add support for the XDG spec? I don't see usage of the expected env. variable... Or does the Path module handle this as expected?

[dkvc]

Thanks for the new PR! I'm bit confused thought. Shouldn't this PR add support for the XDG spec? I don't see usage of the expected env. variable... Or does the Path module handle this as expected?

The Path module handles it automatically. This PR actually updates the old os.path.expanduser to pathlib.Path.expanduser. The main benefit of this PR is removing redundant DEFAULT_CONFIG_PATH and adding pathlib.Path.expanduser; the latter automatically is updated version of old one and uses environment variables set by user/default.

[AuHau]

Hmmm, I don't know about it. Quick googling nor the documentation did not confirm that the pathlib.Path.expanduser should follow XDG spec. Do you have a machine that uses XDG? Can you confirm that when the XDG_CONFIG_HOME is set it changes the behavior?

[dkvc]

Yes, you're correct. I'm sorry. It looks like neither pathlib.Path.expanduser nor os.path.expanduser follows XDG spec for HOME config. They only check if $HOME env is set or uses default variable. In most cases, this is fine since most Linux distros set $XDG_CONFIG_HOME as empty or root directory.

Env variables on Podman container:

Env variables on default Fedora installation:

There is still an option to check if it is set, by checking if it is set. But what would happen if it defaults to root directory as in Podman container (First image)? 😨

[AuHau]

It looks like neither pathlib.Path.expanduser nor os.path.expanduser follows XDG spec for HOME config.

Yeah, I thought so. That is the reason why #126 was created...

There is still an option to check if it is set, by checking if it is set. But what would happen if it defaults to root directory as in Podman container (First image)?

Yeah I would go with the route to check if it is set and if so, to use that otherwise, fall back to the pathlib.Path.expanduser()...

IMHO, it is not our problem as to what is the XDG_CONFIG_HOME set to. By the XDG spec this is a place where "Where user-specific configurations should be written", if it is set to root, then it is a problem of the OS environment or users configuration... Or maybe there is some reason for it... Who knows...

[dkvc]

By the XDG spec this is a place where "Where user-specific configurations should be written", if it is set to root, then it is a problem of the OS environment or users configuration... Or maybe there is some reason for it... Who knows...

Sorry, it was only that specific container.. Hence, we can just check if XDG_CONFIG_HOME is set. If it is set, use it; else default to $HOME.

There you go. Done!

[AuHau]

Nice! Thanks a lot!

[AuHau]

Actually 🙈 I thought about it a bit more and realize that this is a breaking change, which we can and should avoid in this form.

The problem is that existing users have their configuration under ~/.togglrc. For users that have configured XDG_CONFIG_HOME and updated toggl-cli to the new version with this change, toggl-cli will look for configuration under $XDG_CONFIG_HOME/.togglrc, which will fail because it does not exist. But it exists under ~/.togglrc... Which will break things.

I would therefore suggest this solution. If XDG_CONFIG_HOME is configured, and $XDG_CONFIG_HOME/.togglrc does not exists, fallback to ~/.togglrc. We could also log a warning for this case.

We could potentially move the configuration from the old place to the new one, but IMHO, that is too proactive and magical behavior, and I would leave the move up to the user so he is aware of that.

[dkvc]

If XDG_CONFIG_HOME is configured, and $XDG_CONFIG_HOME/.togglrc does not exists, fallback to ~/.togglrc. We could also log a warning for this case.

Yes, I didn't consider this possibility. I think this sentence is misinterpreting or I might be wrong;
If $XDG_CONFIG_HOME is configured and ~/.togglrc doesn't exist, then use $XDG_CONFIG_HOME/.togglrc. Else fallback to ~/.togglrc.

[StanczakDominik]

Yeah, fallback to trying ~/.togglrc if we tried $XDG_CONFIG_HOME and that didn't exist 😄

[dkvc]

Yeah, fallback to trying ~/.togglrc if we tried $XDG_CONFIG_HOME and that didn't exist 😄

Wait, I'm confused. Aren't we creating $XDG_CONFIG_HOME/.togglrc?.
It does mean we have to check if ~/.togglrc exists, right?

Edit: This should be better I guess. If 👍 , I will push the changes.

_file_path = Path.expanduser(Path('~/.togglrc'))
    if "XDG_CONFIG_HOME" in os.environ and not _file_path.exists():
        DEFAULT_CONFIG_PATH = Path(os.environ["XDG_CONFIG_HOME"]).joinpath(".togglrc")
    else:
        DEFAULT_CONFIG_PATH = _file_path

[AuHau]

Your solution is almost there, but not completely. The cases I want to cover are these:

So basically $XDG_CONFIG_HOME/.togglrc is used only if it exists or no .togglrc (eq. ~/.togglrc) exists yet (and XDG_CONFIG_HOME is configured).

[dkvc]

I suppose this should be able to cover all the mentioned edge cases. Shall warnings.warn be added to describe about this modification?

_old_file_path = Path.expanduser(Path('~/.togglrc'))
    
    if "XDG_CONFIG_HOME" in os.environ:
        _new_file_path = Path(os.environ["XDG_CONFIG_HOME"]).joinpath(".togglrc")
        
        if _new_file_path.exists() or not _old_file_path.exists():
            DEFAULT_CONFIG_PATH = _new_file_path
        else:
            DEFAULT_CONFIG_PATH = _old_file_path
            
    else:
        DEFAULT_CONFIG_PATH = _old_file_path

[AuHau]

Yes, that looks about right! 👍

[AuHau]

LGTM! Thanks for the tweaks! 👍