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

Parse complex optargs with sequential types #682

Closed
wants to merge 3 commits into from

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Jan 9, 2018

This patch enables to parse options using complex optargs like SYM:VER=PATH
with types that are not nested but straight up sequential, like [sym str str].

@arichiardi arichiardi force-pushed the fix-multi-separator-cli branch 2 times, most recently from 5fffc8a to 41824a3 Compare January 9, 2018 21:43
(boot.util/info "Testing against version %s\n" (App/config "BOOT_VERSION"))
(comp (runtests)
(deftask integration-test []
(set-env! :source-paths #{"integration-test"})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am always tripping on this one. It is not working, will fix it.

@arichiardi arichiardi force-pushed the fix-multi-separator-cli branch 2 times, most recently from dd0b064 to 1600b5f Compare January 10, 2018 03:50
@arichiardi
Copy link
Contributor Author

arichiardi commented Jan 10, 2018

Don't understand why the tests are failing in this very weird way:

FAIL in boot.cli-test/cli-tests (cli_test.clj:43)                               
parse-cli
[fix #578] split arguments with two different separators using vector optarg.
expected: {:parent [prj "1.3" "parent"]}
  actual: {:parent [ prj "1.3" "parent"]}
    diff: - {:parent [prj]}
          + {:parent [ prj]}

What is the space there I can't understand, but I will 👍

@arichiardi arichiardi force-pushed the fix-multi-separator-cli branch 2 times, most recently from 32b5af4 to 1bf6038 Compare January 10, 2018 20:29
@arichiardi arichiardi changed the title WIP - [Fix #544] Correctly parse arguments with multiple separators Fix parsing of complex optargs with sequential types Jan 10, 2018
@arichiardi arichiardi changed the title Fix parsing of complex optargs with sequential types Parse complex optargs with sequential types Jan 10, 2018
This patch enables to parse options using complex optargs like SYM:VER=PATH
with types that are not nested but straight up sequential, like [sym str str].
Copy link
Member

@martinklepsch martinklepsch left a comment

Choose a reason for hiding this comment

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

Looks good to me. I haven't needed something but given that tests are added and this doesn't break existing behavior we're probably fine to add it.

Two minor suggestions / questions.

Also there is some documentation which seems to suggest that this existed before? Or maybe I'm confusing things? At the end here: https://github.com/boot-clj/boot/wiki/Task-Options-DSL#complex-options

Please make sure this is up to date & add an example if it feels appropriate.

CHANGES.md Outdated
@@ -34,6 +34,7 @@ and `technomancy` for the above explanation.
- Bump [pomegranate](https://github.com/cemerick/pomegranate) and [dynapath](https://github.com/tobias/dynapath) to `1.0.0`. [#612][612]
- Digest `java.io.File` instead `String` path of jar at sift-action `:add-jar` method [#678][678]
- Add warning about improper use of repl :eval option [#666][666]
- Parse complex optargs with sequential types [682][682]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an example here what was previously possible and whats possible now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will add one

But the typical usage is:

((parse-fn SYM:VER=PATH) \"prj:1.3=parent\") => [prj 1.3 parent]"
[optarg]
Copy link
Member

@martinklepsch martinklepsch Jan 12, 2018

Choose a reason for hiding this comment

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

Would this also work?

((parse-fn SYM:VER=PATH=) \"prj:1.3=parent=more-stuff\")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably no because the parsing needs A-Z interleaved by symbols at the moment

Copy link
Member

@martinklepsch martinklepsch Jan 12, 2018

Choose a reason for hiding this comment

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

Err, I made a typo I meant this:

((parse-fn SYM:VER=PATH=MORE) \"prj:1.3=parent=more-stuff\")

Question is if this pattern can repeat indefinitely or if there is some limit to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it works, I tried it by having a test, one second I am going to push it

This is actually just a small test plus some test refactoring that will
introduce the concept of integration vs unit test (now done with
boot-alt-test).
@arichiardi arichiardi force-pushed the fix-multi-separator-cli branch 2 times, most recently from 868f8dd to 8b5ff32 Compare January 12, 2018 18:33
@@ -34,6 +34,7 @@ and `technomancy` for the above explanation.
- Bump [pomegranate](https://github.com/cemerick/pomegranate) and [dynapath](https://github.com/tobias/dynapath) to `1.0.0`. [#612][612]
- Digest `java.io.File` instead `String` path of jar at sift-action `:add-jar` method [#678][678]
- Add warning about improper use of repl :eval option [#666][666]
- Parse complex optargs with sequential types, see [wiki section][sequential-optargs] [#682][682]
Copy link
Member

Choose a reason for hiding this comment

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

That's perfect 👌

@martinklepsch
Copy link
Member

Merged 197f991 69e2957 9b21dd9

@martinklepsch martinklepsch deleted the fix-multi-separator-cli branch January 13, 2018 11:21
@martinklepsch
Copy link
Member

I need to revisit my workflow for merging PRs bit annoyed that this is picked up as closed instead of merged now. 😏

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