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
Conversation
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 |
Restarted the build, and it looked like it passed that time. We get TravisCI hiccups in Islandoraland, and that usually works. |
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, |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
Iterable
implies that we can produce multiple iterators by callingIterable::iterator
several times. We don't want to make that claim.Stream
is considerably heavier thanIterator
. I'm very much in favor ofStream
becoming more common in our code, but when we need only a very tight set of operations (and the majority of uses offlatMap
are either alone or with afilter
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 Stream
s at a low level, but I am saying that we shouldn't always make Stream
s at a low level. Fundamentally, Stream
represents a computation, but Iterator
represents a flow of elements, and that's really what we have here.
This looks good to me. @awoods any comments? |
* @author ajs6f | ||
* @since 10/9/14 | ||
*/ | ||
public class BlankNodeRdfContext extends NodeRdfContext { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link, @acoburn.
http://article.gmane.org/gmane.comp.version-control.git/217
deab4fc
to
fe242fc
Compare
Resolved with: 89d2826 |
No description provided.