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

Changes to config file location #111

Merged
merged 4 commits into from
Dec 31, 2021

Conversation

chrisroat
Copy link
Contributor

Change config file directory. Default directory changed from within site-packages to /.config/brainglobe . User can override with env var BRAINGLOBE_CONFIG_DIR.

Add config dir parameter to BrainGlobeAtlas

Fixed bug in config.write_config_value where config file was written to default location even if custom path passed in.

This change allows the caller to override the config directory in code, which takes precedence over the default location and the BRAINGLOBE_CONFIG_DIR variables. Clients can use this to set the config dir via their command-line flags.

Default directory changed from within site-packages to <HOME>/.config/brainglobe .   User can override with env var BRAINGLOBE_CONFIG_DIR.
Fixed bug in config.write_config_value where config file was written to default location even if custom path passed in.

This change allows the caller to override the config directory in code, which takes precedence over the default location and the BRAINGLOBE_CONFIG_DIR variables.  Clients can use this to set the config dir via their command-line flags.
@adamltyson
Copy link
Member

Thanks @chrisroat! This looks fine to me. If you could just ensure the tests pass - I think you'll just need to run black:

pip install -e .[dev]
black ./

Happy to merge when you are @vigji, @FedeClaudi.

@chrisroat
Copy link
Contributor Author

Added commit to blacken.

@vigji
Copy link
Member

vigji commented Dec 31, 2021

Looks good to me! Definitively better this way. Thanks @chrisroat, and sorry for the delayed review! Good to merge for me

Copy link
Member

@vigji vigji left a comment

Choose a reason for hiding this comment

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

just an import to remove and then good to go!

bg_atlasapi/config.py Outdated Show resolved Hide resolved
@chrisroat
Copy link
Contributor Author

flaked

@adamltyson
Copy link
Member

Happy to merge once @FedeClaudi has OK'd it.

@FedeClaudi
Copy link
Contributor

looks good to me

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

4 participants