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

[NETBEANS-189] Updates for Sql autocomplete and [NETBEANS-5831] Create a SQL Standard Quoter for Use with Connectionless Cases #3074

Merged
merged 2 commits into from
Jan 15, 2022

Conversation

ebresie
Copy link
Contributor

@ebresie ebresie commented Jul 23, 2021

This allows autocompletion to be possible when no connection is yet established.

This is a clean PR based on #2820

@neilcsmith-net neilcsmith-net added this to the NB13 milestone Nov 11, 2021
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Please have a look here:

https://github.com/matthiasblaesing/netbeans/tree/pr-3074

I added a second changeset (matthiasblaesing@c517348) on top of yours (matthiasblaesing@5288810).

I still have doubts, that there is enough benefit for the work, but I think it should work.

String message = NbBundle.getMessage(SQLCompletionProvider.class, "MSG_NotConnected");
StatusDisplayer.getDefault().setStatusText(message);
return 0;
}
SQLExecutionBaseAction.notifyNoDatabaseConnection();
Copy link
Contributor

Choose a reason for hiding this comment

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

This (and the change in line 88) causes excessive dialogs to popup. In the case of line 92, you even get two dialogs (one telling you there is no connection and the second querying the user credentials).

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've not seen the excessive dialogs mentioned but I will look again and try out what is proposed.

@ebresie
Copy link
Contributor Author

ebresie commented Dec 5, 2021

In your updates, I noticed the addition of a FallbackQuoter in the SQLIdentifiers.java. This may solve the intent of the child ticket "NETBEANS-5831 Create a SQL Standard Quoter for Use with Connectionless Cases" raised during initial development. Would it be worth linking that up with that or referencing it as well (kill two birds with one stone)? Or maybe NETBEANS-5831 should be rejected and just link the changes back to the original NETBANS-189 ticket.

@ebresie
Copy link
Contributor Author

ebresie commented Jan 9, 2022

Probably need some git/github help regarding integration of @matthiasblaesing suggested changes mentioned here:

https://github.com/matthiasblaesing/netbeans/tree/pr-3074

I added a second changeset (matthiasblaesing@c517348) on top of yours (matthiasblaesing@5288810).

I pulled the above code locally (merged/addressed conflicts) and seems to work well locally so far, but what is the proper way to merge/commit/push these in? In previous attempts when I've attempted this some differences/newer upstream changes showed unnecessary (upstream) changes in the PR (i.e. this is why my earlier PR was closed with this "clean PR" instead). I'm trying to avoid this here.

Is it possible in github to "merge matthias" pr into this PR in some way? Is a new PR with all the changes needed?

Or should I merge in the upstream changes (maybe with a rebase), then bring in changes from matthias PR and commit them (to my local branch) then push to my sql branch which assume will update this pr? Or wIll this result in some unrelevant (i.e. upstream changes) being included in the PR as well (hoping the rebase may reduce this but not sure)?

If I merge them in, does that then credit me as the changes in the history? What is the correct way to merge these and ensure proper credit (i.e. Mattias changes) get recorded in the history?

@matthiasblaesing
Copy link
Contributor

If you look at the branch I referenced I have squashed your two commits into one (matthiasblaesing@5288810). In that commit you see, that you are referenced as the author and I'm referenced as the committer, which is correct for this case.

To get this PR onto the current master state, this is what I would do:

  1. Ensure your own repository is registered as a remote with the name origin
  2. Ensure the apache netbeans repository is registered as a remove with the name github

That will result in this (adapt the names for you):

matthias@enterprise:~/src/netbeans$ git remote -v
github	git@github.com:apache/netbeans.git (fetch)
github	git@github.com:apache/netbeans.git (push)
origin	git@github.com:matthiasblaesing/netbeans.git (fetch)
origin	git@github.com:matthiasblaesing/netbeans.git (push)
matthias@enterprise:~/src/netbeans$
  1. Update your local copy of the master branch git fetch github
  2. Fetch my branch:
# Checkout the base commit of my branch (this is where my branch an master diverge)
git checkout -b blaesing-sql_autocomplete_clean f6634f96135ccd5aeae12c68e3f17d4bba268419
# Pull the commits from my branch into the freshly created branch
git pull https://github.com/matthiasblaesing/netbeans.git pr-3074
  1. Checkout your work branch git checkout sql_autocomplete_clean
  2. Rebase it onto current master by running git rebase github/master
  3. To squash the last two commits together, run git rebase -i HEAD~2. An editor window will open, where the two commits will be shown and where you can now change history (literally). The comment below the history gives info what you can do. You want to change pick in the second line to f (short for fixup). This will squash the second commit into the first and only the commit message of the first commit will be retained. Save and exit the editor.
  4. git will now tell you, that the rebase was successful

At this point the changes from this PR are on on top of the current state of master and squashed into a single commit.

  1. Find the commit ID of my commit by running git log -n 5 --oneline blaesing-sql_autocomplete_clean (show the last 5 commits from my branch, the first is the right one)
  2. Cherry pick that commit into your branch git cherry-pick c5173486d454
  3. This introduces a merge conflict in ide/db/apichanges.xml and ide/db/nbproject/project.properties. Open the apichanges.xml file, move the new entry to the top and fix the XML structure, save and exit the editor. Please also change the minor version of the change to 83. For the ide/db/nbproject/project.properties retain the newer spec version.
  4. Mark the conflicts as fixed: git add ide/db/apichanges.xml ide/db/nbproject/project.properties
  5. Finish cherry-picking: git cherry-pick --continue

Now check the result for example using gitk or git log -n 3. The latter should show the first commit to be authored by me, the second by you and the third should show the commit of current master.

Now you need to do a forced push to update this PR:

  1. Run: git push -f

@ebresie
Copy link
Contributor Author

ebresie commented Jan 14, 2022

  1. This introduces a merge conflict in ide/db/apichanges.xml and ide/db/nbproject/project.properties. Open the apichanges.xml file, move the new entry to the top and fix the XML structure, save and exit the editor. Please also change the minor version of the change to 83. For the ide/db/nbproject/project.properties retain the newer spec version.

Due to other DB changes, the minor version was changed to 84

@ebresie
Copy link
Contributor Author

ebresie commented Jan 14, 2022

Merged and build successful. Unless otherwise indicated, ready for inclusion.

@ebresie ebresie changed the title [NETBEANS-189] Updates for Sql autocomplete [NETBEANS-189] Updates for Sql autocomplete and [NETBEANS-5831] Create a SQL Standard Quoter for Use with Connectionless Cases Jan 14, 2022
@matthiasblaesing
Copy link
Contributor

Thank you for the rebase. Looks clean and sane to me. Lets get this in.

@matthiasblaesing matthiasblaesing merged commit 176197a into apache:master Jan 15, 2022
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.

4 participants