fix a few things in Lang and RDFLanguages#1146
fix a few things in Lang and RDFLanguages#1146bvosburgh-tq wants to merge 1 commit intoapache:mainfrom
Conversation
bvosburgh-tq
commented
Dec 29, 2021
bvosburgh-tq
commented
- 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
- 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
|
|
||
| /** Make sure the registration does not overlap or interfere with an existing registration. */ | ||
| private static void checkRegistration(Lang lang) { | ||
| if ( lang == null ) |
There was a problem hiding this 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.
| 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 |
There was a problem hiding this comment.
This is an internal consistency check. Let's leave it.
| Lang otherLang = (Lang)other ; | ||
| return | ||
| this.label == otherLang.label && | ||
| this.altLabels.equals(otherLang.altLabels) && |
There was a problem hiding this 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?
| } | ||
| } | ||
|
|
||
| private static boolean isMimeTypeRegistered(Lang lang) { |
There was a problem hiding this 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.
|
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: Commit messages should correspond to the changes. "fix a few things" will be unclear in a years time! |
|
Could you provide some background and explanation please. The description is too brief. There can be only one There are no new test cases or test case changes. |
|
I've cherry-picked some fixes which are now on Closing this PR - no response to comments, nor about the background and whether there is some functionality change being requested here. One |
|
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 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. : ) |
|
Adding a new writer example : https://github.com/apache/jena/blob/main/jena-examples/src/main/java/arq/examples/riot/ExRIOT7_AddNewWriter.java |
|
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 |
2 participants
