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

Jump to bottom

Soften some edges on help text behavior (rebooted) #611

Merged
merged 7 commits into from Dec 31, 2020
Merged

Soften some edges on help text behavior (rebooted) #611

merged 7 commits into from Dec 31, 2020

Conversation

florisla
Copy link
Contributor

@florisla florisla commented Jan 7, 2019 •
edited

This is a rebase of #580, with added tests.

The original #580 failed on Travis due to code formatting (black).

New tests:

Program
    help
        per parameter
            displays when help is supplied with dashes in key
            displays when help is supplied with underscores in key

Collection
    to_contexts
        raises ValueError on help without matching parameter

Fixes #398 Bug report: no error on mismatched help keys and task arguments
Fixes #409 Bug report: help text on parameters with underscore
Closes #533 Duplicate bug report to #409
Closes #590 Duplicate feature request to #409
Closes #580 Original merge request

@florisla
Copy link
Contributor Author

florisla commented Jan 7, 2019

Sorry for opening so many pull requests recently, it's not my goal to overload the maintainer(s) with work.

My main motivation was fixing #409 and the others are just attempts to get some low-hanging-fruit issues closed.

@florisla
Copy link
Contributor Author

Rebased on master since I spotted #409 was in the 'Next bugfix' milestone.

Plug: #607 would be a nice one to include as well.

@bitprophet bitprophet added this to the Next bugfix milestone Jan 30, 2020
This was referenced Dec 30, 2020
Closed
Closed
haydenflinner
haydenflinner reviewed Dec 30, 2020
View reviewed changes
invoke/tasks.py
opts["help"] = self.help[name]
help_name_key = name if name in self.help else opts.get("attr_name")
if help_name_key in self.help:
opts["help"] = self.help.pop(help_name_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

On line 61 above, we could perhaps use self.help = help.copy() if help else {} rather than help directly, in case someone is passing the same help dictionary to multiple decorators to avoid copy-pasting

Copy link
Member

Choose a reason for hiding this comment

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

About to read all this over myself, but I think I agree; that sort of defensive/functional-style programming is something I am frequently worrying about but not always finding the time to actually implement 🙃

@bitprophet
Copy link
Member

Looking at this now with an eye for getting it out in 1.5.

This was referenced Dec 31, 2020
Closed
Closed
bitprophet added a commit that referenced this pull request Dec 31, 2020
@bitprophet
Changelog re #409, re #398, re #611, re #580
3a2613c
bitprophet added a commit that referenced this pull request Dec 31, 2020
@bitprophet
Tweaks re #611
cfc0c29 
- Simplify implementation a tad, arguably
- Streamline tests and make them more unit-y, no real reason to be integration-style here
- Copy help arg to prevent surprise mutation
@bitprophet bitprophet merged commit fb717cf into pyinvoke:master Dec 31, 2020
@Allu2 Allu2 mentioned this pull request May 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bitprophet bitprophet bitprophet left review comments

@haydenflinner haydenflinner haydenflinner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Next bugfix
3 participants
@florisla @bitprophet @haydenflinner