-
-
Notifications
You must be signed in to change notification settings - Fork 354
Add git-config include. #345
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
Conversation
@Byron plz take a look when taking a break from multithreading :-) |
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 took a look mostly at the tests this time, with a few general comments.
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 a lot!
I think the biggest change still needed is to pass in context so interpolate(…)
has something to work with.
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 really like seeing how this PR is developing, and includeIf
is probably in reach now, too.
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 fixes!
Some more ideas came up, I think they may help prepare the code for includeIf
as well.
My plan is to merge this one next time.
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 was about to prepare this PR for merging when I noticed one last thing that I couldn't fix in the little time that remained.
Once it is addressed I'd love to merge. Thanks.
So close :)! When refactoring and looking at the code I noticed that it would by its very nature loose the include sections themselves. Maybe I am wrong about this, but could you add a test to assure that doesn't happen? I double-checked with Thank you. |
As discussed, two more small items on the list of the ever expanding scope.
|
Ready may be? :-) |
- Add git-config include for absolute paths. - Temp disable criterion due to cargo-deny (advisories). - Pass git_install_dir when creating config. Canonicalize include paths. - Use `from_paths::Error`. - Fix clippy warnings.
…max include depth is exceeded.
That way, it's more easily discoverable should people search for it.
It was born in the `from_paths()` method which eventually showed that it wasn't possible to easily resolve paths in an existing config file, as is the case in the `from_env()` constructor. There still is a TODO asking how to resolve relative include paths if there was no path on disk.
Thanks a lot for your contribution, it's merged! Please note also about the comment in 3cbe072, I think it would be valuable to split tests into integration tests without affecting the API, and factor the unit tests out into their own module files (without being integration tests). Even though this refactoring could be done at any time, I recommend doing it before proceeding with |
Awesome & thanks for the valuable guidance while working on it! Yes, I was unsure about the tests and will act on the comment in 3cbe072 |
#331