Export rdflib NamedNodes #3
Conversation
RubenVerborgh
left a comment
RubenVerborgh
left a comment
There was a problem hiding this comment.
Thanks for your work on this. However, this PR reduces the compatibility of solid-namespace from all RDF/JS-compatible libraries to just rdflib.js. This does not seem an enhancement to me, hence my non-approval.
README.md
Outdated
|
|
||
| A collection of common RDF namespaces used in the Solid project. | ||
|
|
||
| solid-namespace is meant to be used with [rdflib.js](/linkeddata/rdflib.js). It exports a collection of `NamedNode` values for commonly used namespaces, thereby shortcutting the need to call `$rdf.namespace(...)`. |
There was a problem hiding this comment.
Meant to be used with any RDF/JS-compatible library, including rdflib.js.
package.json
Outdated
| "rdf": "^4.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "standard": "^12.0.1" |
There was a problem hiding this comment.
Out of scope for this PR?
This returns things similar to how they were functioning in 0.1.0 but keeps all the additional namespaces and reduces the dependencies
|
@RubenVerborgh I've reworked my fork to work as described above and updated the README and JS doc comments etc. Let me know what you think! |
There was a problem hiding this comment.
That's one step, but 'Namespace is not in RDF/JS.
What we really need is the following:
- if no RDF library provided, return concatenated string
- if RDF library provided, return NamedNode
So, given your const aliases (renamed to prefixes), we need (draft and untested):
return function (rdf = { namedNode: u => u }) {
const namespaces = {};
for (const prefix in prefixes) {
const expansion = prefixes[prefix];
namespaces[prefix] = function (localName = '') {
return rdf.namedNode(expansion + localName);
};
};
return namespaces;
};More elaborate version at https://github.com/rdfjs/N3.js/blob/v1.0.0-beta.2/lib/N3Util.js#L42
|
Interesting.. didn't realize that Namespace wasn't part of RDF/JS! I totally botched the exporting of an object when no rdflib is given too. I'll get this eventually! 😑 |
They might be a bit foo focused on rdflib (from my perspective, opinions vary).
Hey, thanks for persisting :-) The result is going to be something better than either of us would've done alone. |
… intended behavior :D
Definitely! Have a look now.. I also added tests to avoid my botching something obvious again :D |
RubenVerborgh
left a comment
RubenVerborgh
left a comment
There was a problem hiding this comment.
Love it, thanks for the tests as well. Minor nit: I think we're following standard here, so no semis (but I realized I introduced them).
|
So how do we get this PR actually merged? Am I supposed to do something at this point? |
|
I don't have publish permissions on solid-namepaces, so I can merge but not publish. Next time, could you please not bump the version? Because package maintainers usually have their own process for doing that. |
👍 understood, that makes total sense. |
2 participants
This is for #2. See the issue for more discussion.