Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix rejection of negating CLI boolean flags in config #1913

Merged
merged 10 commits into from
Jul 25, 2023
Merged

Fix rejection of negating CLI boolean flags in config #1913

merged 10 commits into from
Jul 25, 2023

Conversation

chrysle
Copy link
Contributor

@chrysle chrysle commented Jul 16, 2023 •
edited by atugushev
Loading

Closes #1909

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

@chrysle chrysle added the bug Something is not working label Jul 16, 2023
@chrysle chrysle force-pushed the support-boolean-flags-in-config branch from 37664bf to 401c5fd Compare July 16, 2023 13:33
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
piptools/utils.py Outdated Show resolved Hide resolved
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
piptools/utils.py Outdated Show resolved Hide resolved
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
piptools/utils.py Outdated Show resolved Hide resolved
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
tests/test_utils.py Outdated Show resolved Hide resolved
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

@chrysle thank you very much for fixing this! I've left a few comments above for code improvement.

@atugushev atugushev added the config Related to pip-tools' configuration label Jul 16, 2023
@chrysle
Copy link
Contributor Author

chrysle commented Jul 16, 2023

Thanks for the review! I've tried to apply your suggestions.

@atugushev atugushev mentioned this pull request Jul 24, 2023
Merged
@atugushev
Copy link
Member

atugushev commented Jul 24, 2023 •
edited
Loading

@chrysle please look into the chrysle#1, I've made some changes to clean up the code a bit.

atugushev and others added 2 commits July 24, 2023 18:56
@atugushev
Refactor-out parse_config_file() (#1)
2a91b76 
* Refactor-out parse_config_file()

* Fix failing tests

* Naming

* Naming
@chrysle
Merge branch 'main' into support-boolean-flags-in-config
61eda61
@atugushev
Copy link
Member

atugushev commented Jul 24, 2023 •
edited
Loading

@chrysle would you like to document that feature in the README?

@chrysle
Add documentation to README
1d1976e 
And correct README filename in `tox` session.
@chrysle
Copy link
Contributor Author

chrysle commented Jul 24, 2023

Sure, I added a small paragraph.

atugushev
atugushev reviewed Jul 24, 2023
View reviewed changes
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Thanks for the docs! A few comments to consider below:

README.md Outdated
Comment on lines 305 to 313
Since `pip-tools` supports default values vor [all valid command-line flags](/cli/index.md)
of its subcommands, you can also achieve the functionality in the above example
by setting a configuration value for the negative flag `--no-annotate`:

```toml
[tool.pip-tools]
no-annotate = false
```

Copy link
Member

@atugushev atugushev Jul 24, 2023 •
edited by chrysle
Loading

Choose a reason for hiding this comment

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

This may seem like a special case. However, we essentially support any long options (those starting with -- prefix) listed in command line reference represented in either snake or kebab case (without the double dash). Perhaps it's worth mentioning this instead? What do you think?

Copy link
Contributor Author

@chrysle chrysle Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Apparently, the Sphinx TOML lexer doesn't seem to concur with that suggestion...
Perhaps it isn't such a good idea to support both cases, then?

Copy link
Member

Choose a reason for hiding this comment

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

See #1913 (comment)

Copy link
Contributor Author

@chrysle chrysle Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've tested that configuration though, and seen it work...

Copy link
Member

@atugushev atugushev Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It might work, since we do the normalization before the validation for now. However, we should not encourage this, because it will be fixed soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may be able to call _validate_config() directly in _invert_negative_bool_options_in_config().

README.md Outdated Show resolved Hide resolved
@chrysle
Copy link
Contributor Author

chrysle commented Jul 24, 2023

Thanks, I agree with your points and reworked the documentation.

atugushev
atugushev reviewed Jul 24, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@chrysle
Copy link
Contributor Author

chrysle commented Jul 25, 2023

Sorry for the misunderstanding, I hope it's better now.

atugushev
atugushev approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Thanks @chrysle! 🎉

@atugushev atugushev merged commit 9aa6ec4 into jazzband:main Jul 25, 2023
34 checks passed
@chrysle chrysle deleted the support-boolean-flags-in-config branch July 25, 2023 17:31
@atugushev
Copy link
Member 

Bug Fixes:

- Add support for negating CLI boolean flags in config (https://github.com/jazzband/pip-tools/pull/1913). Thanks @chrysle

@chrysle a side-note about changelog: "Add support" usually goes to "Features" section. Do you think it would make sense to either rephrase the PR title or reconsider its labels?

@chrysle
Copy link
Contributor Author

chrysle commented Jul 26, 2023

Thanks for pointing out! How about:

Bug Fixes:

- Fix rejection of negating CLI boolean flags in config (https://github.com/jazzband/pip-tools/pull/1913). Thanks @chrysle

@atugushev atugushev changed the title Add support for negating CLI boolean flags in config Fix rejection of negating CLI boolean flags in config Jul 26, 2023
@atugushev atugushev added this to the 7.2.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atugushev atugushev atugushev approved these changes

Assignees
No one assigned
Labels
bug Something is not working config Related to pip-tools' configuration
Projects
None yet
Milestone
7.2.0
Development

Successfully merging this pull request may close these issues.

Using negative flag forms (e.g. no-header, no-annotate) in config has no effect
2 participants
@chrysle @atugushev