Skip to content

Fix rejection of negating CLI boolean flags in config - #1913

Merged
atugushev merged 10 commits into
jazzband:mainfrom
chrysle:support-boolean-flags-in-config
Jul 25, 2023
Merged

Fix rejection of negating CLI boolean flags in config#1913
atugushev merged 10 commits into
jazzband:mainfrom
chrysle:support-boolean-flags-in-config

Conversation

@chrysle

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

Copy link
Copy Markdown
Contributor

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
Comment thread piptools/utils.py Outdated
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
Comment thread piptools/utils.py Outdated
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
Comment thread piptools/utils.py Outdated
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
Comment thread tests/test_cli_compile.py Outdated
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
Comment thread tests/test_utils.py Outdated
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes
Comment thread tests/test_cli_compile.py Outdated
atugushev
atugushev reviewed Jul 16, 2023
View reviewed changes

@atugushev atugushev left a comment

Copy link
Copy Markdown
Member

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

chrysle commented Jul 16, 2023

Copy link
Copy Markdown
Contributor Author

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

@atugushev atugushev mentioned this pull request Jul 24, 2023
Merged
@atugushev

atugushev commented Jul 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@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

atugushev commented Jul 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@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

chrysle commented Jul 24, 2023

Copy link
Copy Markdown
Contributor Author

Sure, I added a small paragraph.

atugushev
atugushev reviewed Jul 24, 2023
View reviewed changes

@atugushev atugushev left a comment

Copy link
Copy Markdown
Member

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:

Comment thread 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
```

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

Copy link
Copy Markdown
Member

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?

@chrysle chrysle Jul 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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?

@atugushev atugushev Jul 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See #1913 (comment)

@chrysle chrysle Jul 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@atugushev atugushev Jul 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@chrysle chrysle Jul 25, 2023

Copy link
Copy Markdown
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().

Comment thread README.md Outdated
@chrysle

chrysle commented Jul 24, 2023

Copy link
Copy Markdown
Contributor Author

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

atugushev
atugushev reviewed Jul 24, 2023
View reviewed changes
Comment thread README.md
@chrysle

chrysle commented Jul 25, 2023

Copy link
Copy Markdown
Contributor Author

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

atugushev
atugushev approved these changes Jul 25, 2023
View reviewed changes

@atugushev atugushev left a comment

Copy link
Copy Markdown
Member

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
@chrysle
chrysle deleted the support-boolean-flags-in-config branch July 25, 2023 17:31
@atugushev

atugushev commented Jul 25, 2023

Copy link
Copy Markdown
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

chrysle commented Jul 26, 2023

Copy link
Copy Markdown
Contributor Author

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