Skip to content

Place config & data files according to XDG standarts#557

Closed
dsseng wants to merge 2 commits intocli:trunkfrom
dsseng:fix/xdg-config-home
Closed

Place config & data files according to XDG standarts#557
dsseng wants to merge 2 commits intocli:trunkfrom
dsseng:fix/xdg-config-home

Conversation

@dsseng
Copy link
Copy Markdown
Contributor

@dsseng dsseng commented Feb 27, 2020

Fixes #554

mislav
mislav suggested changes Feb 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mislav mislav left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your contribution! Unfortunately, it's not this easy, otherwise we would have already done this ourselves.

If we wanted to have XDG basedir spec compliance, we would need to implement the entire spec. Specifically, your PR doesn't address these requirements:

  1. In addition to XDG_CONFIG_HOME, the list of paths in XDG_CONFIG_DIRS (including its default value) should also be searched to find the configuration file.
  2. The config.yml file is not the only file that gh writes. There is also the ~/.config/gh/state.yml data file, which should get placed under XDG_DATA_HOME, since it's not a configuration file.
  3. In addition to XDG_DATA_HOME, the list of paths in XDG_DATA_DIRS (including its default value) should also get searched.

It might be a good idea to search for a Go library that implements all this, instead of implementing it yourself from scratch. This one looks like a good one: https://github.com/OpenPeeDeeP/xdg

Also, some additional considerations:

  • The XDG basedir spec is designed for Linux. Since GitHub CLI is cross-platform, how should lookup of these paths be implemented cross-platform, specifically on Windows?
  • There are already existing users of GitHub CLI with existing configurations in a set place (~/.config/gh/*). If support for XDG basedir spec ships in a future release, this might change the location where configuration is looked up for those users, and their old configuration will be discarded. Should we put some code in place that migrates configuration files from old to new location, or will we force users to re-authenticate?

@dsseng
Copy link
Copy Markdown
Contributor Author

dsseng commented Feb 27, 2020

@mislav adrg/xdg seems suitable. It can search for files in suitable directories automatically. What do you think about this one?

@mislav
Copy link
Copy Markdown
Contributor

mislav commented Feb 28, 2020

adrg/xdg seems suitable. It can search for files in suitable directories automatically. What do you think about this one?

The one I've linked to looks better written to me. It doesn't use vars to set up everything important on the package level, for starters, which I generally find an antipattern in Go.

@dsseng
Copy link
Copy Markdown
Contributor Author

dsseng commented Feb 28, 2020

I checked it now, it has Query* methods too, so I'll use it. Thanks!

@dsseng dsseng requested a review from mislav February 28, 2020 11:11
@dsseng
Copy link
Copy Markdown
Contributor Author

dsseng commented Feb 28, 2020

@mislav I implemented this using OpenPeeDeeP/xdg

@dsseng
Copy link
Copy Markdown
Contributor Author

dsseng commented Feb 28, 2020

Also I've got an offtopic question: I've seen a note in contributing guide that you aren't accepting feature PRs now, but I've seen some of those PRs. Are features accepted now?

@dsseng dsseng changed the title Respect XDG_CONFIG_HOME for config directory Place config & data files according to XDG standarts Feb 28, 2020
@muesli
Copy link
Copy Markdown
Contributor

muesli commented Mar 4, 2020 •
edited
Loading

May I recommend using https://github.com/muesli/go-app-paths/ ? It handles this kind of situation across all platforms, for Unix, macOS and Windows based systems alike.

Simple showcase:

import (
        gap "github.com/muesli/go-app-paths"
)

...
scope := gap.NewScope(gap.User, "github", "github")
config, err := scope.ConfigPath("cli.conf")

This results in config being set to (variable portions being already correctly expanded):
Unix/Linux: ${XDG_CONFIG_HOME}/github/cli.conf
macOS: ~/Library/Preferences/github/cli.conf
Windows: %LOCALAPPDATA%/github/Config/cli.conf

@dsseng
Copy link
Copy Markdown
Contributor Author

dsseng commented Mar 4, 2020

Hmm, sounds great @muesli ! I'd like to use this one. Thank you. @mislav what do you think about it?

@muesli
Copy link
Copy Markdown
Contributor

muesli commented Mar 4, 2020 •
edited
Loading

Let me add that go-app-paths does not support XDG_CONFIG_DIRS yet, but other than that follows the XDG specs. I'm happy to add support and fully support XDG in that regard, though, should it be a requirement.

@mislav
Copy link
Copy Markdown
Contributor

mislav commented Mar 6, 2020

Let me add that go-app-paths does not support XDG_CONFIG_DIRS yet, but other than that follows the XDG specs.

Your project looks nice, but I would say it's a must to support the *_DIRS part of the spec. Otherwise we're advertising XDG spec support but we are not completely following it.

@dsseng
Copy link
Copy Markdown
Contributor Author

dsseng commented Mar 6, 2020 •
edited
Loading

@mislav What do you think about current implementation on this branch?

@muesli
Copy link
Copy Markdown
Contributor

muesli commented Mar 6, 2020

Your project looks nice, but I would say it's a must to support the *_DIRS part of the spec. Otherwise we're advertising XDG spec support but we are not completely following it.

Sure, let me add that this weekend, I probably have a personal use-case for it now, as well.

@mislav
Copy link
Copy Markdown
Contributor

mislav commented Mar 6, 2020

@sh7dm I will conduct thorough review next week! Thank you for your patience 🙏

@muesli
Copy link
Copy Markdown
Contributor

muesli commented Mar 13, 2020

@sh7dm Just a heads up: I've updated go-app-paths to be fully XDG-compliant.

@mislav mislav changed the base branch from master to trunk May 27, 2020 11:41
@vilmibm
Copy link
Copy Markdown
Contributor

vilmibm commented Jun 4, 2020

Apologies for letting this go to seed. This fell into the category of "we want it but failed to make time to stop and incorporate" given the changes we've been making to gh's configuration system since March.

Given that this PR is now out of date and it sounds like go-app-paths might be the preferable library, I'm going to close this PR. I invite it to be re-opened (or replaced with another one) that takes another stab at XDG compliance if someone wants to take that on.

@vilmibm vilmibm closed this Jun 4, 2020
@oracle3987

This comment has been minimized.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mislav mislav Awaiting requested review from mislav

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improved XDG basedir spec compliance

5 participants

@dsseng @mislav @muesli @vilmibm @oracle3987