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.

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

Make verify-all respect disabled and overriden commands #22

Merged
merged 3 commits into from
Aug 26, 2022

Conversation

jacksmith15
Copy link
Collaborator

@jacksmith15 jacksmith15 commented Aug 23, 2022

Related issue: #20

This PR changes the verify-all command, so that overriden and disabled commands are respected by it.

It also adds a configuration option so that users can fine-tune what's included, the ordering, and add entirely new commands.

I think parts of this logic should be applied to other command groups (e.g. lint), but I'm not sure it should be part of this PR, as there are differences in how we might want to solve it. For example the lint group could just match every command called lint-*? Or it could have a separate config option, but that might get messy.

@jacksmith15 jacksmith15 requested a review from radeklat August 23, 2022 11:15
@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #22 (1506b3c) into main (9aa0f69) will decrease coverage by 0.12%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   48.77%   48.65%   -0.13%     
==========================================
  Files          20       20              
  Lines         695      707      +12     
  Branches       94       97       +3     
==========================================
+ Hits          339      344       +5     
- Misses        353      360       +7     
  Partials        3        3              
Flag Coverage Δ
integration_tests 5.79% <11.11%> (+0.04%) ⬆️
total 48.51% <55.55%> (-0.12%) ⬇️
unit_tests 48.51% <55.55%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/delfino/click_utils/command.py 75.00% <40.00%> (-8.34%) ⬇️
src/delfino/commands/verify_all.py 68.42% <54.54%> (-14.92%) ⬇️
src/delfino/models/pyproject_toml.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Owner

@radeklat radeklat left a comment

Choose a reason for hiding this comment

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

This could be generalized to allow arbitrary groups. But I think this is a great first step. In case of lint, I think being explicit is better. Another option could be introducing a decorator to mark commands that are part of the same group. But I would prefer to keep this minimal and not turn it into another invoke.

@radeklat radeklat merged commit 047f818 into main Aug 26, 2022
@radeklat radeklat deleted the feat/smart-verify-all branch August 26, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants