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 10:42:32 UTC
[GitHub] [jena] afs commented on a change in pull request #1146: fix a few things in Lang and RDFLanguages
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