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

Add shell command integration #61

Merged
merged 28 commits into from
Oct 15, 2016
Merged

Add shell command integration #61

merged 28 commits into from
Oct 15, 2016

Conversation

eliangcs
Copy link
Contributor

@eliangcs eliangcs commented Jun 27, 2016

  • Execute any strings in backticks as shell commands
  • Shell command lexer (syntax highlighting)
  • Shell command completer
  • Sync with master and resolve conflicts

@eliangcs eliangcs changed the title Add Shell command integration Add shell command integration Jun 27, 2016
@fogine
Copy link
Contributor

fogine commented Aug 23, 2016

Hello @eliangcs ,

do you have some thoughts on how the "Shell command completer" should work?
I mean, have you been planning on using an additional library for bash completion?
Personally, I don't think bash completion is needed here...

@eliangcs
Copy link
Contributor Author

@fogine I agree with you that bash completion is not needed here at the moment. But we do want to make sure the original autocomplete feature won't be broken by the backtick syntax.

@fogine
Copy link
Contributor

fogine commented Aug 24, 2016

Also, is it ok if I push the "pipe to shell command" redirection to this PR, @eliangcs ?
For example:

localhost> post | `tee /tmp/req.log <<< $data` # $data variable would be available with the httpie response here

Or it could automatically redirect the httpie output to stdin of first bash command in the bash expression, eg.:

localhost> post | `tee /tmp/req.log`

The bash expression of the above command would be automatically resolved to:

echo $httpieResponse | tee /tmp/req.log

Which is less flexible but preserves the bash-like "pipe" functionallity

@eliangcs
Copy link
Contributor Author

eliangcs commented Aug 24, 2016

@fogine I'm not sure if I understand your question correctly. But in my original design, the content quoted with backticks is different from the content coming after the first pipe char (|).

Anything that is quoted in backticks should be preprocessed in shell environment before the whole command is executed by http-prompt. So for example, if you do:

http://localhost> post | `head -n 2 | grep name`

It should prompt the user for the input for the head -n 2 | grep name part. Suppose the user enters:

name 123
abc

Then the head -n 2 | grep name part should be evaluated to name 123, so the whole command would be same as doing:

http://localhost> post | name 123

On the other hand, anything coming after the first pipe char should be treated as a shell command that takes the previous http-prompt output as stdin. For example:

http://localhost> post | head -n 2 | grep name

It should make the post request and get the response first. Then put the response output to head -n 2 | grep name.

@fogine
Copy link
Contributor

fogine commented Aug 24, 2016

@eliangcs , to be honest, I haven't known about the special behavior of a code quoted with backticks in the bash, I have been always using the $() syntax. The backticks fits better here.

The way how you described your bash-like design makes sense and is more intuitive, thanks for the explanation.

ps: I won't have time to implement the remaining functionality till next week...

@fogine
Copy link
Contributor

fogine commented Aug 30, 2016

I started implementing "pipe to shell command redirection" as we discussed above.
HERE is the PEG grammar definition for it.

May I ask you, @eliangcs , do you know how "rule precedence" work in the PEG module?
I found an issue where the author of the parsimonious module states:

Precedence in PEGs is expressed by the order of rule nesting

I don't quite understand the statement.
Is it possible to have the shell_cmd_redir rule always executed/processed at the end (as last rule) ?
So that if I do:

https://api.github.com> get | tee /tmp/test

...the get request will be always sent first before the execution of the shell_cmd_redir rule - that is before thetee /tmp/test command.
I've found operator precedence table in PEG specification... however I struggle with getting it work...

Do you know if it's possible with a PEG parser?

Thank you for your time @eliangcs !

@eliangcs
Copy link
Contributor Author

eliangcs commented Sep 1, 2016

@fogine Can you explain more? Take get | tee /tmp/test for example. How does your current grammar behave? And how do you want it to behave?

@fogine
Copy link
Contributor

fogine commented Sep 1, 2016

@eliangcs , in the ExecutionVisitor rule-specific methods are hooked up to the class (eg.: visit_somerule(self, node, children).

After the Parsimonious module finishes parsing a command, it runs matching rules in order.
Eg.: visit_immutation is called first and then other nested matching rules...
When I get get | tee /tmp/test input I want always execute the get request as the first matching rule and then spawn shell subprocess with the request response.

However I think I get it now. What I haven't realized is that currently, the visit_immutation method is handling action / preview requests... which is the root of node tree therefore the method is called at the end (after nested matching rules' methods which are hooked up to the ExecutionVisitor class).
if I rewrite the ExecutionVisitor so that preview / action commands will be processed within the visit_action and visit_preview methods, I can then attain the desired functionality of processing the action / preview rule first, before the shell command redirection part (| tee /tmp/test)

@eliangcs
Copy link
Contributor Author

eliangcs commented Oct 8, 2016

@fogine I just finished this PR along with your patch. Would you like to review?

Copy link
Contributor

@fogine fogine left a comment

Choose a reason for hiding this comment

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

Excellent work @eliangcs !

The code has got much prettier.

I've not found any bugs so far...
Just two mentioned minor things from my observation...
Thanks.

p = Popen(cmd, shell=True, stdout=PIPE)
return p.stdout.read().decode('utf-8').rstrip()

def _is_backticks_cmd_preceded_with_pipe_redir(self, node):
Copy link
Contributor

@fogine fogine Oct 8, 2016

Choose a reason for hiding this comment

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

The _is_backticks_cmd_preceded_with_pipe_redir method is no longer needed (unused).

@@ -169,6 +184,12 @@ def __init__(self, context, listener=None):
self.tool = None
self._output = Printer()

# If there's a pipe, as in "httpe post | sed s/POST/GET/", this
Copy link
Contributor

Choose a reason for hiding this comment

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

just a typo :) "httpe"

@eliangcs
Copy link
Contributor Author

eliangcs commented Oct 8, 2016

@fogine I've fixed the two things you mentioned. Good catch, thanks!

@eliangcs eliangcs merged commit 941b06a into master Oct 15, 2016
@eliangcs eliangcs deleted the issue/54-shell-command branch October 15, 2016 04:11
@eliangcs eliangcs mentioned this pull request Oct 17, 2016
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