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

Adding interactive mode #38

Closed
wants to merge 10 commits into from
Closed

Conversation

Code-Hex
Copy link
Contributor

What I did?

  • Added interactive mode
  • Fixed so that it can be executed without passing it as a string argument. like gitql select hash, author, message from commits

@Code-Hex
Copy link
Contributor Author

Code-Hex commented Dec 16, 2016

I creating functions such as complementary functions and settings, so please do not merge yet.

@Code-Hex Code-Hex mentioned this pull request Dec 16, 2016
@luizperes
Copy link
Collaborator

Good job! I will check it @Code-Hex

@luizperes
Copy link
Collaborator

luizperes commented Dec 19, 2016

Hi @Code-Hex, please check the PR I sent to you: Code-Hex#1 in order to fix your conflicts and add the latest features.

By the way, I checked your files and they are working properly. :) However, we will need to solve the conflicts first

@Code-Hex
Copy link
Contributor Author

@luizperes Thanks for the conflict was fixed.

@luizperes
Copy link
Collaborator

No problem! :) I've already checked and it seems to be working properly, I'm just waiting now for @cloudson review and approval! Thanks @Code-Hex.

Ah, before I forget, I think that we should discuss the number of project dependencies, because as far as I know this PR includes 3 new ones, right?

@filhodanuvem
Copy link
Owner

Incredible job @Code-Hex 👏 🎆 🤘
The feature is working perfectly, I'm just worried with some points:

  • As @luizperes said, you put new libraries into the project. Beyond chzyer/readline is the other one indispensable for you? Our installation is not already too fast so we need of the minimal number of dependencies as possible (we need to remove packages like go-clitable soon)

  • I saw a little difference on the flags usage
    On the master gitql -type works but after this change it doesn't (gitql --type works).
    I liked this change, long flags should to have two dashes (-) but I think that it could to be in a separated commit/PR, it can break someone using the flag and micro changes make our life more calm 😆
    What do you guys think?

@Code-Hex
Copy link
Contributor Author

@cloudson Thank you very much!!

As @luizperes said, you put new libraries into the project. Beyond chzyer/readline is the other one indispensable for you? Our installation is not already too fast so we need of the minimal number of dependencies as possible (we need to remove packages like go-clitable soon)

I understand the desire to make your installation faster.
I believe that you want to speed up the installation for binary distribution.
For me, Rather than using go get, cross compiling will be done and binaries will be distributed. like this.
I thought that it would be better for you to think about installing go get for developers.

I think that it could to be in a separated commit/PR, it can break someone using the flag and micro changes make our life more calm 😆

That's good! Rather I should have done so 😓

@luizperes
Copy link
Collaborator

Hi, I believe that I was the one who included the --type flag to this PR, my bad @Code-Hex and @cloudson

@Code-Hex
Copy link
Contributor Author

Code-Hex commented Dec 23, 2016

@cloudson @luizperes I'm sorry and I was late because I was busy.
It was completed for the time being. However, because we are terminating the program around runtime, we often forcibly terminate during interactive mode.
But it should be fixed with another Pull Request or Branch 👍

@Code-Hex
Copy link
Contributor Author

Since the completion function was not enough, I committed it again.
Please code review them. Thank you 😄


func suggestTokensFromInputting(focus []rune, pos int) [][]rune {
return suggestInputting(focus, pos, [][]rune{
[]rune("select"),
Copy link
Owner

Choose a reason for hiding this comment

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

We need to declare the reserved words as constants on lexical package to use these there and here.
This pull request is too big already so please, let us work in another PR.

// gitql> select * from commits where committer = "K" order by com[tab
// gitql> select * from commits where committer = "K" or com[tab
// In the case is inputting column inputted after "where", "by", "and", "or"
return suggestColumnsFromInputting(focus, pos)
Copy link
Owner

@filhodanuvem filhodanuvem Dec 24, 2016

Choose a reason for hiding this comment

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

I tried to type
select message from commits where tab or
select mesage from commits where date > '2017-01-01 and tab

And I saw the suggestions for reserved word instead of columns. It would be nice put unit tests for the functions, I can help you.

@filhodanuvem
Copy link
Owner

@Code-Hex I'm really impressived with your work here. Thank you again.
This pull request itself was really big and it was causing worries for me, so I've merged all the changes before the autocomplete commit (by the way, it is an awesome improvement).
Can you please make a rebase on your develop branch and make a new pull request just about autocomplete stuffs? I hope help you to write some unit tests on there.

@Code-Hex
Copy link
Contributor Author

@cloudson OK!
Thank you for merge!!

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.

3 participants