basho / clique

CLI Framework for Erlang
Apache License 2.0
147 stars 49 forks source link

Fix kv parsing to allow equal sign in value #86

Closed baranga closed 5 years ago

baranga commented 6 years ago

Allow = in value of kv arguments.

Example: my-cli do-something url=example.com?q=dada

Fix for basho/clique#85

russelldb commented 6 years ago

It looks OK to me.

What would be great is some more steps I can take to verify your work.

Maybe something like

before the change if you do A, B, C you get bad outcome D, now, if you do E, F, G you get good outcome H"

And then I can test it without having to page in all of clique to get started. Reviews and merges happen sooner if you make the reviewers job as simple and quick as possible

Even a line I can cut-and-paste to run your new test would be a real benefit.

Thanks for the work. Hopefully can get it merged today.

baranga commented 6 years ago

I don't know how to come up with a good simple example so I'll just describe why I filed this: As described in erlio/vernemq#740 I try to set a configuration option including a equal sign via CLI (vernemq uses clique for CLI handling):

$ vmq-admin webhooks register hook=auth_on_register endpoint=my-service/hook?key=value
Too many equal signs in argument: "endpoint=my-service/hook?key=value"

For me that looks like a valid use case which should be either supported like this PR implements it or via some escaping scheme.

With this change this would be expected result:

$ vmq-admin webhooks register hook=auth_on_register endpoint=my-service/hook?key=value
Done

It just ignores additional equal signs so they are part of value.

Is that helpful?

codeadict commented 6 years ago

Any update on this one?

russelldb commented 5 years ago

Test passes for me. +1 from me, but I think @martincox or @martinsumner should be the ones to pull the trigger and merge.

baranga commented 5 years ago

@martincox @martinsumner Let me know if I can help with anything to get this merged and released.

martincox commented 5 years ago

lgtm. :+1:

martincox commented 5 years ago

Changed target to develop-3.0 - as that's the active branch.