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

Document full path of config path better #626

Closed
wants to merge 1 commit into from
Closed

Conversation

Flimm
Copy link

@Flimm Flimm commented Mar 30, 2023

This is a small tweak. With the old documentation, I created a file named /home/song/.config/python_keyring and I wondered why it failed silently. I realise that I should have read the existing documentation more carefully. I think this change will make it even easier for users to find the configuration file.

On some systems with Python 3, only the python3 executable is available (and not python), so I changed the executable to python3 as well. This project doesn't support Python 2 any more.

@jaraco
Copy link
Owner

jaraco commented May 22, 2023

I think this change will make it even easier for users to find the configuration file.

Escaping the filename is useful. I like that.

Adding the os.path.join is somewhat helpful (providing the full path), at the expense of making the command significantly longer (and thus more difficult to type if needed). Isn't it preferable since the name has already been prescribed to simply help the user locate the containing directory?

On some systems with Python 3, only the python3 executable is available (and not python), so I changed the executable to python3 as well. This project doesn't support Python 2 any more.

I'd really like to get away from the aberration which is "python3". That name doesn't exist on Windows. I'd consider using py (python-launcher) as that exists on both platforms. Otherwise, I'd prefer to stick to the version-agnostic name (python), which can (and in my opinion should) refer to the best available Python.

@Flimm
Copy link
Author

Flimm commented May 23, 2023

This was my first-time experience. I ran this command that I found in the documentation:

$ python3 -c "import keyring.util.platform_; print(keyring.util.platform_.config_root())"
/home/flimm/.config/python_keyring

I then tried to create a file named python_keyring in the .config directory. I didn't read the previous paragraph in the documentation properly. It took me a while to understand why my custom configuration was having no effect. I contributed this proposed change to make the documentation harder to misunderstand in the way that I did.

With regards to running python3 or python, Python's documentation says that the binary is expected to be python3. However, on Windows, python3 doesn't exist. I've asked a question about this on Stack Overflow: https://stackoverflow.com/q/75885628/247696

@jaraco
Copy link
Owner

jaraco commented May 29, 2023

Probably if we want to make it easy to emit the entire config path, that logic should probably be embedded in the library, rather than adding long recipes for copy paste in the docs. Maybe a keyring diagnose command or keyring config command.

@Flimm
Copy link
Author

Flimm commented May 30, 2023

That's fine by me. I think my pull request is minor and doesn't merit more time spent on discussion. I'm happy for you to make the final call.

@jaraco jaraco mentioned this pull request Jun 16, 2023
@jaraco
Copy link
Owner

jaraco commented Jun 16, 2023

Thanks for understanding. I'll decline this change for the reasons stated above, but plan to follow up with something better in #633.

@jaraco jaraco closed this Jun 16, 2023
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.

None yet

2 participants