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

Refactor cli to avoid double args parsing #567

Merged
merged 2 commits into from
Jun 12, 2016

Conversation

mpeterv
Copy link
Contributor

@mpeterv mpeterv commented Jun 3, 2016

Instead of 'run' function command modules now have 'command' function that accepts already parsed flags. Code mutating original args in 'luarocks.command_line' and 'util.forward_flags' are no longer needed.

There was a comment saying that the old 'run' interface, although undocumented, is used by some people. It may be irrelevant now but I've added an utility function wrapping the new interface to provide 'run'. It's called on all command modules.

New command module interface: instead of 'run' function
they must have 'command' function that accepts flags table
and other arguments. For compatibility a new util function
is called on all command modules: it adds 'run' function
that parses command-line args before passing them to 'command'.
Use 'commands' functions directly.
@mpeterv
Copy link
Contributor Author

mpeterv commented Jun 7, 2016

@hishamhm the comment about 'run' interface being used by other programs is four years old, do you know if it's still true?

@Alloyed
Copy link
Contributor

Alloyed commented Jun 11, 2016

afaict it is a reference to
https://github.com/stevedonovan/LuaRocksTools/tree/master/api
which at least on github was last updated 5 years ago, and had major bugs anyways when I tried to use it verbatim.

I use a patched variant to automate luarocks commands in loverocks (example), but I am perfectly okay with my abuse of internal APIs breaking. The proposed patch seems easy enough to feature-detect.

@hishamhm
Copy link
Member

@mpeterv @Alloyed Sounds reasonable! I think we could get rid of the run() function in the LuaRocks 3 branch.

@hishamhm hishamhm merged commit f75295e into luarocks:master Jun 12, 2016
@mpeterv mpeterv deleted the cli-refactor branch June 13, 2016 08:44
@mpeterv
Copy link
Contributor Author

mpeterv commented Jun 13, 2016

@hishamhm done!

@hishamhm
Copy link
Member

awesome, thank you!

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.

None yet

3 participants