-
Notifications
You must be signed in to change notification settings - Fork 11
Replace deprecated appdirs
with platformdirs
, fix config path lookup with non conda
-based environments
#113
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
Replace deprecated appdirs
with platformdirs
, fix config path lookup with non conda
-based environments
#113
Conversation
Should I add a CHANGELOG entry as well here, or will it be picked up by the PR title automatically? It would be nice to get this PR in a release, but there's no hurry :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add a CHANGELOG entry as well here, or will it be picked up by the PR title automatically?
It will be picked up automatically :) Happy to make a patch release with this! Thanks!
@@ -24,7 +24,7 @@ classifiers = [ | |||
"Programming Language :: Python :: 3.11", | |||
] | |||
dependencies = [ | |||
"appdirs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the CI says appdirs is still being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be addressed by emscripten-forge/pyjs-code-runner#15, marking for completeness :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I believe the change is not correct. It does not respect the current conda environment and it installs at the root config dir: '/home/martin/.config/empack'
for me, while it should respect that I'm using micromamba here so /home/martin/micromamba/envs/whatever/share/empack
or /home/martin/micromamba/envs/whatever/.config/empack
I shall take a look. We might need the same changes across https://github.com/emscripten-forge/pyjs-code-runner, as it uses |
This would be by design of |
I'm surprised that sys.prefix isn't set properly when using pyenv, isn't it using the right Python? |
I'm surprised, too, as I just installed
Something else must be at play. |
As we want to ensure that a config file must be tied to the current installation of |
I hope 8739917 is acceptable! I tried it out locally, and it passes the test suite. With this commit, the ```/home/martin/micromamba/envs/whatever/lib/python3.X/site-packages/empack/empack_config.yaml` which, I feel, is better than having to deal with |
pyproject.toml
Outdated
@@ -48,9 +48,6 @@ empack = "empack.cli.main:app" | |||
[project.urls] | |||
Homepage = "https://github.com/emscripten-forge/empack" | |||
|
|||
[tool.hatch.build.targets.wheel.shared-data] | |||
config = "share/empack" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope I won't be too annoying to you on this. I have a preference for this to stay 🫤
I believe we should make sure we can find this path when using pyenv.
It seems jupyter has something like that https://github.com/jupyter/jupyter_core/blob/main/jupyter_core/paths.py#L102 for finding whether or not it should use user-level paths or conda environment paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope I won't be too annoying to you on this.
Absolutely not! :D I'd also love to ensure pyenv
works and not move anything else. Let me port Jupyter's thing here, then.
Okay, so, based on that assessment – this means that usage of Could you check 273966b? I've ported over Jupyter's logic, but I don't know if the license notes would be needed. This seems to resolve the issue locally: >>> from empack.pack import DEFAULT_CONFIG_PATH
>>> DEFAULT_CONFIG_PATH now gets us PosixPath('/Users/agriyakhetarpal/.pyenv/versions/3.10.4/share/empack/empack_config.yaml') while retaining the behaviour required by PosixPath('/opt/homebrew/Caskroom/miniforge/base/share/empack/empack_config.yaml' |
appdirs
with platformdirs
, use platformdirs.user_config_dir
for DEFAULT_CONFIG_PATH
appdirs
with platformdirs
, fix config path with non conda
-based environments
appdirs
with platformdirs
, fix config path with non conda
-based environmentsappdirs
with platformdirs
, fix config path lookup with non conda
-based environments
@agriyakhetarpal it seems you need to run Also how about we install |
Thanks! Addressed both points. |
Sorry, it seems there is a last linting issue 😓 |
Argh, strange that it didn't happen locally 😅 Addressed with 3b3a32e. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description
As the
appdirs
project is no longer maintained (ActiveState/appdirs@8734277), this pull request swaps out the dependency to useplatformdirs
. Additionally, it changesempack
'sDEFAULT_CONFIG_PATH
lookup to also cater to virtual environments that are not managed byconda
or equivalent tools, in light of matplotlib/matplotlib#29506 where a problem was noticed when usingpyenv
-based Python installations/interpreters.Additional context
platformdirs
documentation: https://platformdirs.readthedocs.io/en/stable/