-
Notifications
You must be signed in to change notification settings - Fork 48
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
Refactor user data directories with platformdirs
#122
Conversation
@romainsacchi @tngTUDOR I would love to get your input on the following: The current export directory structure could also be refactored, so that the export directory wouldn't depend on the current working directory. Are you open to such a change? I have a solution in mind where it would be set by the user, by an environment variable, or would default to a platformdirs location. This would also apply to the logs directory, which sits in the export hierarchy. Is there a reason that |
|
premise/__init__.py
Outdated
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.
moves the DATA_DIR related vars to the new filesystem_constants module.
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 changed all the internal imports, and these paths were in __all__
so shouldn't be importable in any case?
check_internal_linking, | ||
link_internal, | ||
) | ||
from wurst.linking import change_db_name, check_internal_linking, link_internal |
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.
which is the right black formatting ?
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.
The CI does black auto-formatting, so we can't make this look any different in any case.
setup.py
Outdated
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.
suggestion: let's keep only 1 source of requirements (duplicated in setup.py
and requirements.txt
)
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.
Don't forget meta.yaml
in conda
:) Not sure that one can be anything but duplicative...
I agree that having a platformdirs folder, is an elegant way of centralizing files. |
The CI failing test is outside the changes I made (pandas variable name). @romainsacchi can I mark it skipped? |
Fixes #100.
The current implementation stores user data in the
site-packages
directory. This is undesired, as:The alternative is to give each user a cache directory in a suitable location. We follow other Brightway libraries and user platformdirs, the successor to
appdirs
.This PR also centralized the location of directory path creation, which should prevent inconsistencies, and provides one clear place where one can find filesystem constants:
filesystem_constants.py
.