-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
Fix revealed default config in header if requirements in subfolder #1904
Conversation
what about this? I think it should pickup config from pyproject.toml |
tests/test_utils.py
Outdated
Regression test for issue GH-1903. | ||
""" | ||
default_config_file = Path("pyproject.toml") | ||
default_config_file.touch() |
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.
Can we also test the case when the [tool.pip-tools]
section is present and empty?
Also, how about a .pip-tools.toml
with an empty or missing [tool.pip-tools]
and pyproject.toml
with a non-empty one? I think that the pyproject.toml
shouldn't be taken into account in this case.
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.
Okay, I'll add those combinations to parametrized tests 👍🏻
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.
Addressed in 64445d0
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 hoping to see what happens if both files exist 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.
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.
This looks like out of scope of get_compile_command
. We can add more detailed unit tests for select_config_file()
directly. What do you think?
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.
If it's out of the scope, it'd be good to add the tests separately.
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.
Addressed In 845029a
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
I guess #1906 is to be merged first? |
I've added more tests, and it turns out that the config option apparently requires eagerness. Otherwise, it would break the compilation without an explicitly passed config. Working on a fix. |
Looks like the bug is fixed now. Ready for another round of review. |
FTR, resolved some conflicts after merging with the main branch. |
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.
Left a minor comment. Otherwise, LGTM!
Co-authored-by: Ryan Walker <[email protected]>
Thanks @webknjaz and @theryanwalker for the review! |
Ensure
src_files
are handled prior to--config
, asoverride_defaults_from_config_file
is dependent onsrc_files
.Fixes #1903.
Contributor checklist
Maintainer checklist
backwards incompatible
,feature
,enhancement
,deprecation
,bug
,dependency
,docs
orskip-changelog
as they determine changelog listing.