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

Using library code in a few classes #171

Merged
merged 4 commits into from
Oct 8, 2016

Conversation

ajs6f
Copy link
Member

@ajs6f ajs6f commented Sep 25, 2016

Just a couple of orthogonal commits that use library code in Collections, switch to using default methods where it seems appropriate, remove assignments with no effect, that sort of thing. If no one has any objection within a couple of days, I'll merge it myself.

@afs
Copy link
Member

afs commented Sep 26, 2016

Given PR #139 in-progress, can we hold back on the JSON-LD changes for now?

@@ -32,52 +38,35 @@
return rand(numRand, low, high, false) ;
}

/** Generate a random sequence between low (inclusive) and high (exclusive) - choose whether to have duplicates or not */
/**
* Generate a random sequence between low (inclusive) and high (exclusive) - with duplicates or no
Copy link
Member

Choose a reason for hiding this comment

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

Truncated javadoc -- was "or not"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@@ -95,19 +84,13 @@
}
if ( !found )
System.err.printf("Corrupted permute: [%s] [%s]\n", strings(x), strings(x2)) ;
}
}*/
Copy link
Member

Choose a reason for hiding this comment

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

Delete commented out code? (we have git!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Glad to!

@ajs6f
Copy link
Member Author

ajs6f commented Sep 27, 2016

Yes, certainly I'll hold off-- I have no idea how I missed that, especially given that I just pointed out that JSON-LD PR to a colleague in a downstream project yesterday!

@@ -23,7 +23,6 @@
import java.io.OutputStreamWriter ;
Copy link
Member

Choose a reason for hiding this comment

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

This has moved to riot/writer/

else
JsonUtils.write(writer, obj) ;
if ( isPretty() ) JsonUtils.writePrettyPrint(writer, obj) ;
else JsonUtils.write(writer, obj) ;
Copy link
Member

@afs afs Oct 6, 2016

Choose a reason for hiding this comment

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

I don't like if statements with their positive clause on the same line unless they have no else clause and are not part of the flow of the logic. (e.g. not part of the flow: if ( DEBUG) System.err.println(....) ;)

Node pred = t.getPredicate();
return pred.equals(RDF.type.asNode()) || ctx.containsKey(pred.getLocalName());
})
.forEachRemaining(addToContext) ;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it was clearer to have the if ( p.equals(RDF.type.asNode()) ) in teh Consumer because all the logic is in one place.

@ajs6f
Copy link
Member Author

ajs6f commented Oct 6, 2016

Now that we have #139 in, I'll add a further commit addressing your points. Thanks!

@ajs6f
Copy link
Member Author

ajs6f commented Oct 6, 2016

Made changes in response to your review, @afs . If you have no further objections, I'll merge this.

@asfgit asfgit merged commit b6d0f61 into apache:master Oct 8, 2016
@ajs6f ajs6f deleted the JsonLDWriterJava8Stuff branch October 8, 2016 14:08
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

3 participants