JENA-1454: Introduce builder pattern for result set reading and writing. #334
Conversation
|
Needs some further cleanup before its ready to merge. The main syntax forms, XML, JSON support read/write with a TSV, CSV, Text work in this framework but have not been deeply integrated. All old mechanisms to access features specific to a syntax should still work. |
| throw new RiotException("No parser registered for content type: "+ct.getContentType()) ; | ||
| return reader.read(in, context) ; | ||
| public static ResultSet parse(String uri, Lang hintLang, Context context) { | ||
| ResultSet rs = ResultsReader.create().lang(hintLang).context(context).read(uri); |
There was a problem hiding this comment.
In the #readAny methods, there's a checkLang call to verify whether the language is not null and supported. Do we need this check in all other public methods that use a Lang object? Or just in the #readAny methods?
There was a problem hiding this comment.
All read public methods of ResultSetMgr that take a Lang use readAny so lang gets checked.
Adding a check to the write methods makes sense (now done in local working copy) because it's necessary (no default included, no legacy requirement).
ResultSetMgrisn't the main API (ResultSetFormatter is, albeit older and clunkier).
For parse, the method is only there because there was one before this upgrade. There isn't a "no lang" version. lang==null makes sense because the uri is used (conneg or file extension).
Some of the if ( rs == null ) checks are actually unnecessary because SPARQLResult does not return null, it throws an exception, but have been left in place in case that changes.
There was a problem hiding this comment.
👍 makes sense. Happy to take another look once you say it's complete and ready to be merged. Thanks for the quick reply.
There was a problem hiding this comment.
I'll add checks to ResultReader and ResultWriter and use try-resource,
|
Cleaning up and switching to new reader/writer code now done. This PR is now complete. |
| throw new NotImplemented(); | ||
| /** Write a boolean result, using the configurartion of the {@code ResultWriter}, to a file */ | ||
| public void write(String filename, boolean booleanResult) { | ||
| Objects.requireNonNull(booleanResult); |
There was a problem hiding this comment.
Only minor nit-pick I found reviewing the commits added after my previous comment was here.
write(String filename, ResultSet resultSet) has requireNonNull(filename), and requireNonNull(resultSet). And in this method the filename is not checked for null. Not sure if it is intentional or not.
There was a problem hiding this comment.
Thanks - fixed. Testing the wrong argument; Objects.requireNonNull(booleanResult) is never null!
|
+1 |
Rework the machinery for ResultSet reading and writing to use a builder pattern (in ResultsReader and ResultsWriter). This is styled after RDFParse/RDFParserBuilder and RDFWriter/RDFWriterBuilder.