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

Simple cleanup using Java 8 types and syntax for RDF generating code #863

Closed
wants to merge 2 commits into from

Conversation

ajs6f
Copy link
Contributor

@ajs6f ajs6f commented Jul 30, 2015

No description provided.

@escowles
Copy link
Contributor

This looks good to me (not sure why one of the Travis builds failed, it builds fine for me), and I've added to it with some more comments and a couple minor tweaks: #865

@ruebot
Copy link
Contributor

ruebot commented Jul 31, 2015

Restarted the build, and it looked like it passed that time. We get TravisCI hiccups in Islandoraland, and that usually works.

@ajs6f
Copy link
Contributor Author

ajs6f commented Jul 31, 2015

I demand higher reliability from the CI product for which I pay nothing!

@@ -359,6 +359,11 @@ public Triple apply(final T prototriple) {
};
}

protected static <From, To> Iterator<To> flatMap(final Iterator<From> i,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than reimplementing a flatmap here (next we'll be implementing map and filter), why not user StreamSupport and the Iterable interface to convert the Iterator into a Stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Iterators::transform and Iterators::filter already exist.

  1. Iterable implies that we can produce multiple iterators by calling Iterable::iterator several times. We don't want to make that claim.
  2. Stream is considerably heavier than Iterator. I'm very much in favor of Stream becoming more common in our code, but when we need only a very tight set of operations (and the majority of uses of flatMap are either alone or with a filter or two) it's not clear to me that it's worth the overhead in code or execution.

I'm not saying we shouldn't ever make Streams at a low level, but I am saying that we shouldn't always make Streams at a low level. Fundamentally, Stream represents a computation, but Iterator represents a flow of elements, and that's really what we have here.

@acoburn
Copy link
Contributor

acoburn commented Aug 3, 2015

This looks good to me. @awoods any comments?

@awoods
Copy link

awoods commented Aug 3, 2015

* @author ajs6f
* @since 10/9/14
*/
public class BlankNodeRdfContext extends NodeRdfContext {
Copy link

Choose a reason for hiding this comment

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

The history for this class has been lost. Instead, you should use:
git mv

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not believe that git mv will preserve the history of this class. Git uses its own heuristic for determining whether a file has been deleted and added somewhere else (e.g. a simple "move") or whether a newly added file is a truly different thing (e.g. a delete and an unrelated addition). If you look at the implementation of SkolemNodeRdfContext, it shares very few similar lines with BlankNodeRdfContext. So, even with git mv, that history is not necessarily going to be saved. The only sure way to keep that history is to rename the file but keep the implementation basically intact (as much as possible) in one commit. In a subsequent commit (that does not get squashed) the implementation can be changed to reflect the Java8 idioms as expressed here.

Personally, I'm not convinced that doing this simply to preserve the history (first a rename with minimal changes to the file followed by a subsequent, more substantive reimplementation) is worth the effort.

Copy link

Choose a reason for hiding this comment

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

I would be very surprised (although happy to be proven wrong) to see that git mv did not save the history in this case. Have you tested your theory, @acoburn?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have encountered similar issues at other times, when a file is renamed and changed substantially. The issue is this: git tracks content, not files.

Copy link

Choose a reason for hiding this comment

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

@awoods
Copy link

awoods commented Aug 4, 2015

Resolved with: 89d2826

@awoods awoods closed this Aug 4, 2015
@awoods awoods deleted the Java8typesForRdfStreams branch August 31, 2015 17:16
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

5 participants