You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by GitBox <gi...@apache.org> on 2021/12/29 00:20:27 UTC
[GitHub] [jena] bvosburgh-tq opened a new pull request #1146: fix a few things in Lang and RDFLanguages
bvosburgh-tq opened a new pull request #1146:
URL: https://github.com/apache/jena/pull/1146
- Lang.equals(Object) did not compare altLabels
- RDFLanguages.register(Lang) unregistered the wrong lang
- RDFLanguages.checkRegistration(Lang) unnecessarily checked mapContentTypeToLang and used the wrong lookup keys to check the altNames, altContentTypes, and fileExtensions
- RDFLanguages.unregister(Lang) did not clear out the altNames
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs closed pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs closed pull request #1146:
URL: https://github.com/apache/jena/pull/1146
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1146:
URL: https://github.com/apache/jena/pull/1146#issuecomment-1002530458
The project uses JIRA: https://issues.apache.org/jira/projects/JENA/issues/.
As it is our record of changes, and people do search it, please open a JIRA and put the JIRA id in the PR title: `JENA-NNNN: text` -- `text` should be suitable for revisiting in a few years time.
Commit messages should correspond to the changes. "fix a few things" will be unclear in a years time!
There can be only one `Lang` for one content type because that's how reader lookup works. Different flavours of writer are handled by `RDFFormat` and `RDFFormatVariant`.
Could you provide some background and explanation please. The description is too brief.
There are no new test cases or test case changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1146:
URL: https://github.com/apache/jena/pull/1146#issuecomment-1009237301
I've cherry-picked some fixes which are now on `main`.
Closing this PR - no response to comments, nor about the background and whether there is some functionality change being requested here.
One `Lang` is one media type. For output, there is`RDFFormat`which takes a `Lang` and a variation name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a change in pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs commented on a change in pull request #1146:
URL: https://github.com/apache/jena/pull/1146#discussion_r776254216
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/RDFLanguages.java
##########
@@ -293,41 +294,27 @@ public static void register(Lang lang) {
}
}
- private static boolean isMimeTypeRegistered(Lang lang) {
- if ( lang == null )
- return false;
- String mimeType = canonicalKey(lang.getHeaderString());
- return mapContentTypeToLang.containsKey(mimeType);
- }
-
/** Make sure the registration does not overlap or interfere with an existing registration. */
private static void checkRegistration(Lang lang) {
- if ( lang == null )
- return;
String label = canonicalKey(lang.getLabel());
Lang existingRegistration = mapLabelToLang.get(label);
if ( existingRegistration == null )
return;
if ( lang.equals(existingRegistration) )
return;
- // Is the content type already registered?
- if ( isMimeTypeRegistered(lang) ) {
- String contentType = lang.getContentType().getContentTypeStr();
- error("Language overlap: " + lang + " and " + mapContentTypeToLang.get(contentType) + " on content type " + contentType);
- return;
- }
+ // any pre-existing mapContentTypeToLang entry has already been removed - no need to check it here
Review comment:
This is an internal consistency check. Let's leave it.
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/Lang.java
##########
@@ -153,6 +153,7 @@ public boolean equals(Object other) {
Lang otherLang = (Lang)other ;
return
this.label == otherLang.label &&
+ this.altLabels.equals(otherLang.altLabels) &&
Review comment:
Not sure it should even be testing alts. A lang corresponds to a MIME type registration which drives reader dispatch.
Has this been an issue for you?
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/RDFLanguages.java
##########
@@ -293,41 +294,27 @@ public static void register(Lang lang) {
}
}
- private static boolean isMimeTypeRegistered(Lang lang) {
- if ( lang == null )
- return false;
- String mimeType = canonicalKey(lang.getHeaderString());
- return mapContentTypeToLang.containsKey(mimeType);
- }
-
/** Make sure the registration does not overlap or interfere with an existing registration. */
private static void checkRegistration(Lang lang) {
- if ( lang == null )
Review comment:
Let's leave this in place. Just because in the current usage, lang is never null does not mean that it was or will be. `checkRegistration` is a robust building block done this way. A piece of defensive code here is zero cost -- it will probably happen before the instruction reaches the head of the execution pipeline and it's on a critical path in Jena anyway.
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/RDFLanguages.java
##########
@@ -293,41 +294,27 @@ public static void register(Lang lang) {
}
}
- private static boolean isMimeTypeRegistered(Lang lang) {
Review comment:
Please leave this function and keep its usage. If it needs fixes,fix it.
Having named operations for specific steps makes the code clearer. The optimizer will deal with efficiency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1146:
URL: https://github.com/apache/jena/pull/1146#issuecomment-1002549319
Could you provide some background and explanation please. The description is too brief.
There can be only one `Lang` for one content type because that's how reader lookup works. Different flavours of writer are handled by `RDFFormat` and `RDFFormatVariant`.
There are no new test cases or test case changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1146:
URL: https://github.com/apache/jena/pull/1146#issuecomment-1010876135
I would also point to SHACL-C in the TQ SHACL engine but you (TQ) have dropped support for that.
Jena registering new reader and writer for SHACL-C : https://github.com/apache/jena/blob/main/jena-shacl/src/main/java/org/apache/jena/shacl/compact/SHACLC.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #1146:
URL: https://github.com/apache/jena/pull/1146#issuecomment-1010872789
Adding a new writer example : https://github.com/apache/jena/blob/main/jena-examples/src/main/java/arq/examples/riot/ExRIOT7_AddNewWriter.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] bvosburgh-tq commented on pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
bvosburgh-tq commented on pull request #1146:
URL: https://github.com/apache/jena/pull/1146#issuecomment-1010678064
I am sorry I blew you off, Andy. : (
I submitted this PR (and 1147) in the middle of a long holiday and it has taken me a while to get back into things.
Thank you for cherry-picking the good stuff from this PR, though!
The thing that instigated this PR was when I worked on a custom writer and was trying to figure out how to register it. There is no clear documentation of how a new writer should be integrated with "RIOT"; so I am left with the code. : ) So, when I looked at the code in `RDFLanguages` to understand what it is for and how I should use it, the code had a few obvious bugs. Thus the PR.
But, although my writer seems to integrate successfully with RIOT, it's still not obvious to me how all the parts (e.g. labels, content types, alt names, alt content types, file extensions) fit together and how they are used by Jena.... : (
Also, I will start with a Jira issue next time. Should be coming soon. : )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs edited a comment on pull request #1146: fix a few things in Lang and RDFLanguages
Posted by GitBox <gi...@apache.org>.
afs edited a comment on pull request #1146:
URL: https://github.com/apache/jena/pull/1146#issuecomment-1002530458
The project uses JIRA: https://issues.apache.org/jira/projects/JENA/issues/.
As it is our record of changes, and people do search it, please open a JIRA and put the JIRA id in the PR title: `JENA-NNNN: text` -- `text` should be suitable for revisiting in a few years time. If it is the PR title, there will automatically be a link to the PR added to the ticket.
Commit messages should correspond to the changes. "fix a few things" will be unclear in a years time!
Having the "JENA-NNNN: ..." for commit messages is also very helpful, both for reviewing the git log and because it causes commit to be added as a comment to the JIRA ticket.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org