Skip to content

Fix revealed default config in header if requirements in subfolder#1904

Merged
atugushev merged 15 commits intojazzband:mainfrom
atugushev:main
Aug 7, 2023
Merged

Fix revealed default config in header if requirements in subfolder#1904
atugushev merged 15 commits intojazzband:mainfrom
atugushev:main

Conversation

@atugushev
Copy link
Copy Markdown
Member

@atugushev atugushev commented Jul 8, 2023 •
edited
Loading

Ensure src_files are handled prior to --config, as override_defaults_from_config_file is dependent on src_files.

Fixes #1903.

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@atugushev atugushev added the bug Something is not working label Jul 8, 2023
@atugushev
Copy link
Copy Markdown
Member Author

atugushev commented Jul 8, 2023 •
edited
Loading

@q0w this should fix your issue too #1902. However, this does not cover "empty config file" case which is not a bug IMO.

@93578237
Copy link
Copy Markdown
Contributor

93578237 commented Jul 8, 2023 •
edited
Loading

what about this? I think it should pickup config from pyproject.toml
#1902 (comment)

@webknjaz webknjaz mentioned this pull request Jul 8, 2023
Merged
4 tasks
@webknjaz webknjaz changed the title Fix revealed default config in header if requirements in suboflder Fix revealed default config in header if requirements in subfolder Jul 9, 2023
webknjaz
webknjaz reviewed Jul 9, 2023
View reviewed changes
Comment thread tests/test_utils.py Outdated
webknjaz
webknjaz reviewed Jul 9, 2023
View reviewed changes
Comment thread tests/test_utils.py Outdated
Regression test for issue GH-1903.
"""
default_config_file = Path("pyproject.toml")
default_config_file.touch()
Copy link
Copy Markdown
Member

@webknjaz webknjaz Jul 9, 2023

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.

Copy link
Copy Markdown
Member Author

@atugushev atugushev Jul 9, 2023

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 👍🏻

Copy link
Copy Markdown
Member Author

@atugushev atugushev Jul 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 64445d0

Copy link
Copy Markdown
Member

@webknjaz webknjaz Jul 9, 2023

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..

Copy link
Copy Markdown
Member Author

@atugushev atugushev Jul 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

@atugushev atugushev Jul 9, 2023 •
edited
Loading

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?

Copy link
Copy Markdown
Member

@webknjaz webknjaz Jul 10, 2023

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.

Copy link
Copy Markdown
Member Author

@atugushev atugushev Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed In 845029a

@chrysle
Copy link
Copy Markdown
Contributor

chrysle commented Jul 11, 2023

I guess #1906 is to be merged first?

@atugushev
Copy link
Copy Markdown
Member Author

atugushev commented Jul 13, 2023 •
edited
Loading

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.

@atugushev atugushev added the config Related to pip-tools' configuration label Jul 16, 2023
@webknjaz webknjaz mentioned this pull request Jul 26, 2023
Merged
@atugushev
Copy link
Copy Markdown
Member Author

atugushev commented Jul 27, 2023

Looks like the bug is fixed now. Ready for another round of review.

@atugushev
Copy link
Copy Markdown
Member Author

atugushev commented Jul 27, 2023

FTR, resolved some conflicts after merging with the main branch.

@atugushev atugushev requested a review from a team July 27, 2023 21:08
theryanwalker
theryanwalker reviewed Aug 7, 2023
View reviewed changes
Comment thread tests/conftest.py Outdated
theryanwalker
theryanwalker approved these changes Aug 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@theryanwalker theryanwalker left a 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!

@atugushev atugushev enabled auto-merge (squash) August 7, 2023 15:17
@atugushev atugushev added this to the 7.3.0 milestone Aug 7, 2023
@atugushev atugushev merged commit d1e9215 into jazzband:main Aug 7, 2023
@atugushev atugushev deleted the main branch August 7, 2023 15:34
@atugushev
Copy link
Copy Markdown
Member Author

atugushev commented Aug 7, 2023

Thanks @webknjaz and @theryanwalker for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@webknjaz webknjaz webknjaz left review comments

@theryanwalker theryanwalker theryanwalker approved these changes

Assignees

No one assigned

Labels

bug Something is not working config Related to pip-tools' configuration

Projects

None yet

Milestone

7.3.0

Development

Successfully merging this pull request may close these issues.

pip-compile reveals default --config=pyproject.toml option in header

5 participants

@atugushev @93578237 @chrysle @webknjaz @theryanwalker