internal/devconfig: move project directory search into devbox.Find #2172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Instead of first searching for a project directory and then calling `devbox.Open` on that path, add a `devbox.Find` method that performs the search and open as a single step. - Keeping `Find` a distinct function from `Open` makes it clearer when we're recursively searching for a config vs. not. - `Find` can just try reading a potential config file path instead of performing a separate `os.Stat`. This halves the number of system calls at startup and eliminates any potential races between the call to `os.Stat` and `os.Open`. ```go // Open loads a Devbox config from a file or project directory. // For use with `devbox -c some/dir <subcmd>`. func Open(path string) (*Config, error) // Find is like [Open] except it recursively searches up the directory tree. // For use with `devbox <subcmd>` (without a `-c` flag). func Find(path string) (*Config, error) ``` Changes to user error messages: ```diff $ devbox add go -Error: No devbox.json found in this directory, or any parent directories. Did you run `devbox init` yet? +Error: no devbox.json found in the current directory (or any parent directories). Did you run `devbox init` yet? $ devbox -c badpath add go -Error: stat /var/folders/79/1yc1ywp10w9f2xnr_rpp_ff00000gn/T/tmp.bU25JVWovO/badpath: no such file or directory + +Error: the devbox config path "badpath" does not exist. $ mkdir child $ devbox -c child add go -Error: No devbox.json found in child. Did you run `devbox init` yet? +Error: no devbox.json found in "child". Did you run `devbox init` yet? ```
mikeland73 approved these changes Jul 8, 2024
| - name: bool-literal-in-expr | ||
| - name: cognitive-complexity | ||
| exclude: | ||
| - "**_test.go" |
| // Open does not recursively search outside of path. See [Find] to load a config | ||
| // by walking up the directory tree. | ||
| func Open(path string) (*Config, error) { | ||
| start := time.Now() |
Collaborator
There was a problem hiding this comment.
If you want to profile performance, you can use defer debug.FunctionTimer().End()
Collaborator
There was a problem hiding this comment.
Maybe we can log that to slog as well?
savil approved these changes Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Some cleanup when I looked into supporting different config extensions (e.g.,
devbox.jsoncto make editors okay with comments).Instead of first searching for a project directory and then calling
devbox.Openon that path, add adevbox.Findmethod that performs the search and open as a single step.FindandOpenfunctions to make it clearer when we're recursively searching for a config vs. not.os.Stat.devbox.jsonwith the-Cflag would fail with a bad error message.Changes to user error messages: