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

resolve JENA-1459 add jena text highlighting #339

Merged
merged 4 commits into from
Jan 17, 2018
Merged

resolve JENA-1459 add jena text highlighting #339

merged 4 commits into from
Jan 17, 2018

Conversation

xristy
Copy link
Contributor

@xristy xristy commented Jan 6, 2018

This PR adds the highlighting feature describe in JENA-1459. This PR includes unit tests for the highlighting feature.

Once the PR is approved, I'll submit an update to the jena-text documentation.

@afs
Copy link
Member

afs commented Jan 15, 2018

There are some conflicts (due to PR #335, I think).

Is this simple to resolve? Is that because this PR includes some of the other one?

@xristy
Copy link
Contributor Author

xristy commented Jan 15, 2018

Good grief!. Yes the conflicts are with PR #335. I apparently merged too fast when I saw the approval for 335 and proceeded with the development of the highlight.

When I issued this PR no conflicts were showing so I thought all was ok. I had prepared a squashed branch per prior requests.

What is the procedure to remedy?

@ajs6f
Copy link
Member

ajs6f commented Jan 15, 2018

It's no big deal-- happens all the time! Do you know how to rebase and resolve conflicts in Git?

https://help.github.com/articles/resolving-merge-conflicts-after-a-git-rebase/
https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/

@xristy
Copy link
Contributor Author

xristy commented Jan 15, 2018

They were simple enough to fix inline via the github tool.

@ajs6f
Copy link
Member

ajs6f commented Jan 15, 2018

Cool. That's essentially an in-line version of the same workflow, minus the rebase. That means you end up with an extra no-real-content commit (c9453b4) but you get where you need to be.

@xristy
Copy link
Contributor Author

xristy commented Jan 15, 2018

I'm glad it was comparatively simple. I was not looking forward to experimenting with rebasing in public -;)

@ajs6f
Copy link
Member

ajs6f commented Jan 15, 2018

Relax-- it's not nearly as bad as all that. 😁 I often say that having to rebase PRs a lot is good-- it means the project is lively and the code is evolving.

For me, the trick (as almost always with git) was to remember that you are making/managing/corralling deltas (aka commits), not versions. A branch can be thought of as a chain of deltas that starts somewhere and ends with a specific commit. Rebasing means taking a chain of commits that ends one branch and swapping what comes before it to be another branch. So in this case, you would have swapped the prefix of your branch (which has all the commits in master except those that were in #335) for master itself. You would have had to do exactly the same adjustments, but you would have ended up with a series of commits that appeared (from the POV of seeing how deltas add up to change) as though you had begun work after #335 merged, which makes for a cleaner public history. Of course your commits always keep their metadata, so we will always actually know when they occurred in clock time.

@xristy
Copy link
Contributor Author

xristy commented Jan 15, 2018

Thanks for the help. I appreciate it.

@asfgit asfgit merged commit c9453b4 into apache:master Jan 17, 2018
asfgit pushed a commit that referenced this pull request Jan 17, 2018
@afs
Copy link
Member

afs commented Jan 17, 2018

Done! (One trivial syntax error fixed.)

@xristy
Copy link
Contributor Author

xristy commented Jan 17, 2018

What was the error? I puzzled that I was not seeing it.

@afs
Copy link
Member

afs commented Jan 17, 2018

TS_Text.java, line 54 had a leading backtick character. I thought I could safely fix it :-)

+` , TestTextHighlighting.class

@xristy
Copy link
Contributor Author

xristy commented Jan 17, 2018

Thanks for fixing. Maybe that got introduced when I "corrected" the conflicts a couple of days ago since I did that inline - I suppose I won't used that approach again, otherwise the tools should have detected it at my end.

@ajs6f
Copy link
Member

ajs6f commented Jan 17, 2018

That is one prob with using the Github on-line editors. But they are convenient! I use them for corrections to human-centric docs like READMEs, etc.

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