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

Update GUIController.java #33

Closed
wants to merge 7 commits into from
Closed

Conversation

YonasJ
Copy link
Contributor

@YonasJ YonasJ commented Jan 29, 2019

For the CoAP server I am connecting to, it returns a hierarchy. And this tool was not including the slashes between path elements, and returning extra errors while browsing the tree. This patch corrects both errors.

For the CoAP server I am connecting to, it returns a hierarchy. And this tool was not including the slashes between path elements, and returning extra errors while browsing the tree. This patch corrects both errors.
@YonasJ
Copy link
Contributor Author

YonasJ commented Jan 29, 2019

Signed the eclipse contribution agreement.

@boaks
Copy link
Contributor

boaks commented Jan 29, 2019

Hi @YonasJ

first thanks for your contribution!
You already passed the first part of the legal boilerplate by signing the ECA.
To pass the "checks", the second part will be signing the commit. That's done by git-signing in the commit message.

Signed-off-by: your name e-mail@e-mail

You must use the e-mail you used for the ECA (yonas.jongkind@gmail.com).

@boaks
Copy link
Contributor

boaks commented Jan 29, 2019

I possible, please add the { ... } and amend the change together with you signing to the commit. The force push them, and we will see, if the ip-validation will work.

Signed-off-by: Yonas Jongkind <yonas.jongkind@gmail.com>
@boaks
Copy link
Contributor

boaks commented Jan 30, 2019

Still thanks!

To pass the ip-validation, please "squash" your 3 commits into 1 commit and ensure, that that one is signed (as you already did for a6699a1). Then just push that commit "forced" to your branch.

YonasJ and others added 2 commits January 30, 2019 09:15
For the CoAP server I am connecting to, it returns a hierarchy. And this tool was not including the slashes between path elements, and returning extra errors while browsing the tree. This patch corrects both errors.
@YonasJ
Copy link
Contributor Author

YonasJ commented Jan 30, 2019

I can't figure out how to do the squash. According to this documentation:
https://help.github.com/articles/merging-a-pull-request/

It seems like there should be an option to squash, but I don't see that menu.

I tried to merge them in the client, but that just seemed to create more commits.

Any documentation about how to do the squash?

YonasJ and others added 2 commits January 30, 2019 09:49
Signed-off-by: Yonas Jongkind <yonas.jongkind@gmail.com>
Add {} per code review request.

Update GUIController.java

For the CoAP server I am connecting to, it returns a hierarchy. And this tool was not including the slashes between path elements, and returning extra errors while browsing the tree. This patch corrects both errors.
@boaks
Copy link
Contributor

boaks commented Jan 31, 2019

Do you use the eclipse IDE?

If so, http://www.vogella.com/tutorials/EclipseGit/article.html contains a lot of useful information. In
http://www.vogella.com/tutorials/EclipseGit/article.html#actions-available-via-the-history-view
you can see, how to use modify-squash.

So, now, after the merge, the 1. thing to do, please "reset --hard" your branch to the commit a6699a1 .
http://www.vogella.com/tutorials/EclipseGit/article.html#moving-the-branch-pointer-with-git-reset
contains the instructions. Then squash your commits to one.

Alternatively:
Right now I pushed a "PR33" branch with the single commit into this eclipse repo.
So fetch the origin, this should create a new remote-branch with name "origin/PR33".
Then reset --hard your branch local branch "patch-1" to that commit 7f37541, and force push this to your repo.

If this doesn't work, and you agree, that I push it to the master branch, I can do this as well :-).
That would show here "closed" instead of "merged", but your commit will be in the master and the comments here will document your PR.

@YonasJ
Copy link
Contributor Author

YonasJ commented Jan 31, 2019

Thanks for all that information.

I made the changes in my eclipse copy of the master repository. However, it's on the trunk and not my own branch.

I made the fix by making the same edit in the "Github" change file UI in the web.

If I had it checked out with Tortoise as i normally do, I think I could squash it. But I can't figure it out for the web version.

If you could accept this patch in this format, that would be great. And next time I will create my own branch and commit the normal way from Eclipse so squashing is easily possible.

@boaks
Copy link
Contributor

boaks commented Jan 31, 2019

Thanks!
I pushed you commit 7f37541 on master.
Please document with a comment here, that the commit is according your changes, then I can close this PR.

@boaks
Copy link
Contributor

boaks commented Jan 31, 2019

Just to mention:
In the meantime, we spend to most time in improving the 2.0.x branch.
May be you consider to use that as well. I pushed your changes also to that branch.

@YonasJ
Copy link
Contributor Author

YonasJ commented Jan 31, 2019 via email

@boaks
Copy link
Contributor

boaks commented Jan 31, 2019

eclipse smarthome

OK, so 1.0.x will be the right one :-).

@boaks boaks closed this Jan 31, 2019
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

2 participants