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

JENA-2273: Fix ASK in ResultSetWriterCSV and -TSV #1189

Merged
merged 1 commit into from
Feb 6, 2022
Merged

Conversation

cygri
Copy link
Contributor

@cygri cygri commented Feb 5, 2022

No description provided.

Copy link
Member

@afs afs left a comment

Choose a reason for hiding this comment

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

Small problem - that code isn't being called.

There is a migration going on from ResultSet to RowSet as the basic for the machinery. RowSet works exclusively at the Node level.

The registration of the ResultSet version is if/false'd out.

ResultSet objects are now an adapter to the RowSet version from package org.apache.jena.riot.rowset.rw as of jena 4.4.0. The code looks very similar.

One style point (which ought to apply to the existing code) on try-finally.
Some tabs are used instead of spaces.
Github display means spaces work better across everyone's views.

@afs
Copy link
Member

afs commented Feb 6, 2022

As long as we're clear this is technically an extension to the standard!

@cygri
Copy link
Contributor Author

cygri commented Feb 6, 2022

Both RowSetWriters had the same problems, and I added a commit that fixes them (and adds the same test classes).

@afs
Copy link
Member

afs commented Feb 6, 2022

I've pulled PR and it all works, command line and in Fuseki but the build itself fails because of missing Apache License headers on new files.

Sorry for not spotting this earlier.

  jena-arq/src/test/java/org/apache/jena/riot/resultset/rw/TestResultSetWriterCSV.java
  jena-arq/src/test/java/org/apache/jena/riot/resultset/rw/TS_ResultSetRIOT_RW.java
  jena-arq/src/test/java/org/apache/jena/riot/resultset/rw/TestResultSetWriterTSV.java
  jena-arq/src/test/java/org/apache/jena/riot/rowset/rw/TestRowSetWriterTSV.java
  jena-arq/src/test/java/org/apache/jena/riot/rowset/rw/TestRowSetWriterCSV.java
  jena-arq/src/test/java/org/apache/jena/riot/rowset/rw/TS_RowSetRIOT_RW.java

It is the same header as a file you've modified.
Touching license headers is something to be done by the contributor, not the project else I'd have fixed them.

I'll leave it up to you whether you want to squash the PR to a single commit or not.

@cygri
Copy link
Contributor Author

cygri commented Feb 6, 2022

I've added the license headers and squashed the commits.

@afs afs merged commit 2d4e707 into apache:main Feb 6, 2022
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.

2 participants