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

Add multiple option for args #70

Merged
merged 8 commits into from
Jun 11, 2022
Merged

Add multiple option for args #70

merged 8 commits into from
Jun 11, 2022

Conversation

nat-n
Copy link
Owner

@nat-n nat-n commented May 30, 2022 •
edited
Loading

WIP solution for #54 @wsascha

The new multiple option can be added to args to make them accept multiple values.

  • only the last positional arg may have multiple set
  • multiple option defaults to false
  • if multiple = true and required = false then the result is nargs = "*", or if required = true then nargs = "+"
  • an integer value >= 2 may also be given to specify an exact number of values are required for an arg
  • for script tasks the values are passed as a list, for command tasks they can be templated into the command (thus potentially passed as multiple arguments), shell tasks don't have any special support.
  • All tasks also get the list of values as an env var in the form of a space delimited string

Still to test:

  • created a task for testing in the scripts project, but still need to write thorough test cases yet in test_script_tasks
  • also need basic test cases for cmd, shell, and sequence tasks
  • what if multiple=true with type=boolean ?

Also todo:

  • Fix other broken tests
  • Document new functionality in the readme
  • Think of a nice way to indicate that an arg accepts multiple arguments when generating documentation for a task

@nat-n nat-n added the enhancement New feature or request label May 30, 2022
@nat-n nat-n force-pushed the feature/54/multiple_value_args branch from 4b4aa9f to e422ff2 Compare June 5, 2022 22:21
wsascha
wsascha reviewed Jun 6, 2022
View reviewed changes
Copy link

@wsascha wsascha left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the late reply, I'm currently on vacation 😅 looks good to me over all. Thanks for your efforts, really appreciated! I left two minor comments below, let me know what you think.

poethepoet/task/args.py
@@ -26,6 +26,7 @@
"positional": (bool, str),
"required": bool,
"type": str,
"multiple": (bool, int),
Copy link

Choose a reason for hiding this comment

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

Not sure if it's really necessary to have an int here. At least I don't know any use cases from tools I used.

Copy link
Owner Author

@nat-n nat-n Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure either. Though it seemed like a low cost feature to support (because it just works with argparse), and imagined that there could be use-cases for it.

tests/fixtures/scripts_project/pyproject.toml
@@ -99,6 +99,29 @@ greet = "pkg:greet"
type = "boolean"


[tool.poe.tasks.multiple-value-args]
Copy link

Choose a reason for hiding this comment

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

Maybe we could also add a sequence task here.

@nat-n nat-n force-pushed the feature/54/multiple_value_args branch from 5add810 to 515467e Compare June 7, 2022 21:45
@nat-n nat-n mentioned this pull request Jun 9, 2022
Closed
@nat-n nat-n force-pushed the feature/54/multiple_value_args branch 2 times, most recently from 7586f86 to 61f3a9f Compare June 10, 2022 20:36
@nat-n nat-n force-pushed the feature/54/multiple_value_args branch from 61f3a9f to b8f82cb Compare June 10, 2022 20:38
@nat-n nat-n force-pushed the feature/54/multiple_value_args branch 2 times, most recently from f098f55 to 6e4d654 Compare June 10, 2022 22:00
@nat-n nat-n force-pushed the feature/54/multiple_value_args branch from 6e4d654 to 798688e Compare June 11, 2022 19:23
@nat-n nat-n force-pushed the feature/54/multiple_value_args branch from fff476c to dba4457 Compare June 11, 2022 21:00
@nat-n nat-n merged commit b891016 into development Jun 11, 2022
@nat-n nat-n deleted the feature/54/multiple_value_args branch June 11, 2022 21:14
nat-n added a commit that referenced this pull request Jun 11, 2022
@nat-n
Add option to specify that a named argument should access multiple va…
78be9bc 
…lues (#70)

Also improve tests for executing on windows

The new `multiple` option can be added to args to make them accept multiple values.

- only the last positional arg may have multiple set
- multiple option defaults to false
- args of type boolean may not have the multiple option set
- if `multiple = true` and `required = false` then the result is `nargs="*"`, or if `required = true` then `nargs="+"`
- an integer value >= 2 may also be given to specify an exact number of values are required for an arg
- for script tasks the values are passed as a list, for command tasks they can be templated into the command (thus
  potentially passed as multiple arguments), shell tasks don't have any special support.
- All tasks also get the list of values as an env var in the form of a space delimited string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wsascha wsascha wsascha left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nat-n @wsascha