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

ENH: "local" config file upward lookup #3953

Closed
neutrinoceros opened this issue Jun 1, 2022 · 5 comments · Fixed by #4205
Closed

ENH: "local" config file upward lookup #3953

neutrinoceros opened this issue Jun 1, 2022 · 5 comments · Fixed by #4205
Labels
enhancement Making something better proposal Proposals for enhancements, changes, etc

Comments

@neutrinoceros
Copy link
Member

Bug report

Bug summary

Since yt 4.0 we support two locations to store a configuration file yt.toml, namely $XDG_CONFIG/yt/yt.toml (this is the global configuration) and ./yt.toml (local)

Now, assuming a data exploration project organised into subfolders, for instance

.
├── scripts
│   ├── exp1
│   │   ├── t1.py
│   │   ├── t2.py
│   │   └── t3.py
│   └── exp2
│       ├── t1.py
│       ├── t2.py
│       └── t3.py
└── yt.toml

The results of any script will differ depending on wether it's launched from the top level of the project (where yt.toml lives) or from within their respective containing directories.

To solve this, we could implement an upward lookup routine to check for yt.toml files in all parents directories until it is found (or we reach root /).

There is a precedent to the proposed behaviour: many tools already implement this mechanism, for instance

@neutrinoceros neutrinoceros added enhancement Making something better proposal Proposals for enhancements, changes, etc labels Jun 1, 2022
@cphyc
Copy link
Member

cphyc commented Jun 1, 2022

I seem to remember we already had this discussion but I cannot find it anymore. Was it on the mailing list?
Maybe here https://mail.python.org/archives/list/yt-users@python.org/message/7E5A4WGVTRAUMVYXOEPHZO5TTE7JWYTQ/?

@neutrinoceros
Copy link
Member Author

I think you're right but I don't know where it happened originally. Anyway I wanted to bring it up again now that #3626 was merged and the code involved is less intricate.

@neutrinoceros
Copy link
Member Author

This patch does what I'm proposing, but I'll refrain from opening a PR for now

diff --git a/yt/utilities/configure.py b/yt/utilities/configure.py
index 4370303d3..961fe9d60 100644
--- a/yt/utilities/configure.py
+++ b/yt/utilities/configure.py
@@ -1,4 +1,5 @@
 import os
+from pathlib import Path
 from typing import Callable, List

 # TODO: import tomllib from the standard library instead in Python >= 3.11
@@ -108,6 +109,15 @@ class YTConfig:

     @staticmethod
     def get_local_config_file():
+        path = Path.cwd()
+        while path.parent is not path:
+            candidate = path.joinpath("yt.toml")
+            if candidate.is_file():
+                print(f"found config at {str(candidate)}")
+                return os.path.abspath(candidate)
+            else:
+                path = path.parent
+
         return os.path.join(os.path.abspath(os.curdir), "yt.toml")

     def __setitem__(self, args, value):

@matthewturk
Copy link
Member

I don't remember why I might have had objections to this, but it seems to me that this is a reasonable thing to do. It's also what git does, right?

@neutrinoceros
Copy link
Member Author

Though not explicitly stated, I think it is, yes.

Finally, Git looks for configuration values in the configuration file in the Git directory (.git/config) of whatever repository you’re currently using
https://www.git-scm.com/book/en/v2/Customizing-Git-Git-Configuration

I'll will open a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better proposal Proposals for enhancements, changes, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants