Skip to content

Add --no-config option#1896

Merged
atugushev merged 4 commits intojazzband:mainfrom
atugushev:no-config
Jul 1, 2023
Merged

Add --no-config option#1896
atugushev merged 4 commits intojazzband:mainfrom
atugushev:no-config

Conversation

@atugushev
Copy link
Copy Markdown
Member

@atugushev atugushev commented Jul 1, 2023

Addresses #1863 (comment).

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 enhancement Improvements to functionality cli Related to command line interface things labels Jul 1, 2023
@atugushev atugushev requested a review from AndydeCleyre July 1, 2023 09:26
Comment thread tests/test_cli_sync.py
dry_run_message = "Would install:"
assert dry_run_message in out.stdout

out = runner.invoke(cli, ["--no-config", "--config", config_file.as_posix()])
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 should be a separate test with its own name or at least a parametrized case..

Copy link
Copy Markdown
Member Author

@atugushev atugushev Jul 1, 2023

Choose a reason for hiding this comment

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

Generally, I agree, however in this case, I prefer the triangulation technique.

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.

I don't see how it's applicable in this case. You're clearly testing two separate behaviors that must appear as separate entries in the test report.
The only way of having two things reported from the same test function is to use pytest-subtest. But it still feels wrong since you're not checking two properties of the same behavior outcome.

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.

Also, for property-based testing, we should really integrate Hypothesis..

Copy link
Copy Markdown
Member Author

@atugushev atugushev Jul 1, 2023

Choose a reason for hiding this comment

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

Okay, here you go 65fe40f

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!

Copy link
Copy Markdown
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Let's try making the test names reflect what it is we're testing...

Comment thread tests/test_cli_compile.py Outdated
Comment thread tests/test_cli_sync.py Outdated
@atugushev atugushev enabled auto-merge (squash) July 1, 2023 10:40
@atugushev atugushev merged commit 407f008 into jazzband:main Jul 1, 2023
@atugushev atugushev deleted the no-config branch July 1, 2023 11:50
@atugushev atugushev added this to the 7.0.0 milestone Jul 14, 2023
@atugushev atugushev added the config Related to pip-tools' configuration label Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Related to command line interface things config Related to pip-tools' configuration enhancement Improvements to functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants