-
Notifications
You must be signed in to change notification settings - Fork 638
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
source_up_if_exists: A strict_env compatible version of source_up #921
Conversation
This fixes direnv#913 While I was in here, I opted to make the existing `source_up` script log explicitly when it can't find a file. I think that's useful both with strict_env enabled and disabled.
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 for the PR!
There are two minor suggestions. If you could also extend the stdlib docs in the man folder that would be perfect.
stdlib.sh
Outdated
local envrc file=${1:-.envrc} | ||
envrc=$(cd .. && (find_up "$file" || true)) | ||
if [[ -n $envrc ]]; then | ||
source_env "$envrc" | ||
fi |
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.
local envrc file=${1:-.envrc} | |
envrc=$(cd .. && (find_up "$file" || true)) | |
if [[ -n $envrc ]]; then | |
source_env "$envrc" | |
fi | |
source_up "${1:-}" || true |
With the above change it becomes possible to deduplicate the logic
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.
That works, but with my change to source_up
, it means you get a red "No ancestor .envrc found" message even though this function still succeeds.
I've tweaked things around a little bit to try to DRY this up. I'm honestly not sure if it's better or worse. What do you think, @zimbatm?
I'm honestly not sure if this is better or worse. See this review feedback discussion for more information: direnv#921 (comment)
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, I pushed a small fixup and it's good to go
Thanks, @zimbatm! |
Thanks for implementing this feature, I was looking for a way to avoid a confusing warning. |
Hi Updated here: f58196a And perhaps the maintainer could add a CI change to validate that the docs source->generated files are up to date (if the generated ones need to be in the repository at all...). update: oops, mis-tagged the author. |
Oh, that's on me, not @zimbatm, sorry! I agree that this feels like an unnecessary footgun: could we get the generated files out of version control? |
This fixes #913
While I was in here, I opted to make the existing
source_up
script logexplicitly when it can't find a file. I think that's useful both with
strict_env enabled and disabled.