Skip to content

Fix #377: Implemented support for adding the tokens to parsed nodes. #388

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Fix #377: Implemented support for adding the tokens to parsed nodes. #388

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a872f8
Implemented support for adding the parser tokens to parsed nodes usin…
faubulous Jul 3, 2024
1e175f7
Added unit test for token enabled DataFactory.
faubulous Jul 3, 2024
1409dd1
Implemented some changes requested by @RubenVerborgh.
faubulous Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 39 additions & 20 deletions src/N3DataFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ let _blankNodeCounter = 0;
const escapedLiteral = /^"(.*".*)(?="[^"]*$)/;

// ## DataFactory singleton
// Note: The default data factory does not set the token field of terms.
const DataFactory = {
namedNode,
blankNode,
variable,
literal,
namedNode: iri => namedNode(iri),
blankNode: name => blankNode(name),
variable: name => variable(name),
literal: (name, datatype) => literal(name, datatype),
Copy link
Member

Choose a reason for hiding this comment

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

I'd be happy leaving these as they are actually. It doesn't hurt to have the second optional argument, and that way, we can expose it to other parsers.

Copy link
Author

@faubulous faubulous Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand you correctly, you want to pass the default constructor functions to the DataFactory as they are. This would mean, however, that the default behavior of the DataFactory changes to always creating Terms with an initialized { token } context. Which in turn will break some tests and is probably not needed in standard situations.

I was hesitant to put a condition into the Parser to check wether to pass the token/context argument to the factory or not because this would have an impact on the performance. The solution to simply ignore the second argument in the DataFactory seemed like a good compromise and allows the API users to define their own behavior regarding the context variable of a node, such as offsetting positions or adding additional data if needed.

So should I revert this to the default functions and have the context always initialized with a token?

Copy link
Member

Choose a reason for hiding this comment

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

the default behavior of the DataFactory changes to always creating Terms with an initialized { token } context.

That's fine; objects can always have optional fields.

Which in turn will break some tests

Those tests might be too strict; we can loosen them.

I was hesitant to put a condition into the Parser to check wether to pass the token/context argument to the factory

Indeed; let's definitely not.

The solution to simply ignore the second argument in the DataFactory

Aah okay. This actually shows me that we need 2 DataFactory classes then. One that discards the context, and one that keeps it. Let's make both first-class citizens.

defaultGraph,
quad,
triple: quad,
Expand Down Expand Up @@ -63,6 +64,12 @@ export class Term {

// ## NamedNode constructor
export class NamedNode extends Term {
constructor(id, token) {
super(id);

this.token = token;
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
constructor(id, token) {
super(id);
this.token = token;
}
constructor(id, token = null) {
super(id);
this.token = token;
}

I'm nitting here, but I want things to be explicit. No undefined.

However, even better would be if the Term constructor simply took token; then we don't need any of this.

And let's not make it token, let's make it context, with let { token } = context.

Copy link
Author

Choose a reason for hiding this comment

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

That's OK. So will the default be context = { token = null } or context = null?

I am personally hesitant to initialize token with an empty string as this is a different datataype as the Token object and it might indicate that one should expect a non-empty string when it is initalized which is misleading.

But I'll do as you wish. Just let me know.

Copy link
Author

@faubulous faubulous Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK. Saw the answer in the comment below. Will go with DEFAULT_CONTEXT.

// ### The term type of this term
get termType() {
return 'NamedNode';
Expand All @@ -71,6 +78,12 @@ export class NamedNode extends Term {

// ## Literal constructor
export class Literal extends Term {
constructor(id, token) {
super(id);

this.token = token;
}

// ### The term type of this term
get termType() {
return 'Literal';
Expand All @@ -92,7 +105,7 @@ export class Literal extends Term {

// ### The datatype IRI of this literal
get datatype() {
return new NamedNode(this.datatypeString);
return new NamedNode(this.datatypeString, this.token);
Copy link
Member

Choose a reason for hiding this comment

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

I'm hesitant to pass the token along here. I don't think we should.

Copy link
Author

Choose a reason for hiding this comment

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

OK.

}

// ### The datatype string of this literal
Expand Down Expand Up @@ -132,8 +145,10 @@ export class Literal extends Term {

// ## BlankNode constructor
export class BlankNode extends Term {
constructor(name) {
constructor(name, token) {
super(`_:${name}`);

this.token = token;
}

// ### The term type of this term
Expand All @@ -148,8 +163,10 @@ export class BlankNode extends Term {
}

export class Variable extends Term {
constructor(name) {
constructor(name, token) {
super(`?${name}`);

this.token = token;
}

// ### The term type of this term
Expand All @@ -167,6 +184,7 @@ export class Variable extends Term {
export class DefaultGraph extends Term {
constructor() {
super('');

return DEFAULTGRAPH || this;
}

Expand All @@ -192,7 +210,7 @@ DEFAULTGRAPH = new DefaultGraph();
// with recursion over nested terms. It should not be used
// by consumers of this library.
// See https://github.com/rdfjs/N3.js/pull/311#discussion_r1061042725
export function termFromId(id, factory, nested) {
export function termFromId(id, factory, nested, token) {
factory = factory || DataFactory;

// Falsy value or empty string indicate the default graph
Expand All @@ -208,7 +226,7 @@ export function termFromId(id, factory, nested) {
case '"':
// Shortcut for internal literals
if (factory === DataFactory)
return new Literal(id);
return new Literal(id, token);
// Literal without datatype or language
if (id[id.length - 1] === '"')
return factory.literal(id.substr(1, id.length - 2));
Expand Down Expand Up @@ -273,7 +291,8 @@ export function termToId(term, nested) {
// ## Quad constructor
export class Quad extends Term {
constructor(subject, predicate, object, graph) {
super('');
super(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Never. Either null or '', but never explicitly undefined.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, no problem.

However, I'm just curious to learn why. Am I right that you want the variable to be initialized with something so that it is visible on an API level? I have to admit that I am not that deep into JavaScript to know all the consequences. It was my reasoning that when passing undefined the variable might not occupy any memory as interpreters might just delete it and thus having the API behave as before.

Copy link
Member

Choose a reason for hiding this comment

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

It was my reasoning that when passing undefined the variable might not occupy any memory

That's not how JavaScript engines work today; the will still explicitly assign the field.

However, I'm just curious to learn why.

Mainly for explicitness. undefined is what happens when we don't pass anything, forget stuff, uninitialized, etc. null signifies a choice. Choices are good.


this._subject = subject;
this._predicate = predicate;
this._object = object;
Expand Down Expand Up @@ -333,20 +352,20 @@ export function unescapeQuotes(id) {
}

// ### Creates an IRI
function namedNode(iri) {
return new NamedNode(iri);
export function namedNode(iri, token) {
return new NamedNode(iri, token);
}

// ### Creates a blank node
function blankNode(name) {
return new BlankNode(name || `n3-${_blankNodeCounter++}`);
export function blankNode(name, token) {
return new BlankNode(name || `n3-${_blankNodeCounter++}`, token);
}

// ### Creates a literal
function literal(value, languageOrDataType) {
export function literal(value, languageOrDataType, token) {
// Create a language-tagged string
if (typeof languageOrDataType === 'string')
return new Literal(`"${value}"@${languageOrDataType.toLowerCase()}`);
return new Literal(`"${value}"@${languageOrDataType.toLowerCase()}`, token);

// Automatically determine datatype for booleans and numbers
let datatype = languageOrDataType ? languageOrDataType.value : '';
Expand All @@ -368,13 +387,13 @@ function literal(value, languageOrDataType) {

// Create a datatyped literal
return (datatype === '' || datatype === xsd.string) ?
new Literal(`"${value}"`) :
new Literal(`"${value}"^^${datatype}`);
new Literal(`"${value}"`, token) :
new Literal(`"${value}"^^${datatype}`, token);
}

// ### Creates a variable
function variable(name) {
return new Variable(name);
export function variable(name, token) {
return new Variable(name, token);
}

// ### Returns the default graph
Expand Down
50 changes: 25 additions & 25 deletions src/N3Parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,19 +159,19 @@ export default class N3Parser {
const iri = this._resolveIRI(token.value);
if (iri === null)
return this._error('Invalid IRI', token);
value = this._namedNode(iri);
value = this._namedNode(iri, token);
break;
// Read a prefixed name
case 'type':
case 'prefixed':
const prefix = this._prefixes[token.prefix];
if (prefix === undefined)
return this._error(`Undefined prefix "${token.prefix}:"`, token);
value = this._namedNode(prefix + token.value);
value = this._namedNode(prefix + token.value, token);
break;
// Read a blank node
case 'blank':
value = this._blankNode(this._prefixes[token.prefix] + token.value);
value = this._blankNode(this._prefixes[token.prefix] + token.value, token);
break;
// Read a variable
case 'var':
Expand All @@ -194,7 +194,7 @@ export default class N3Parser {
case '[':
// Start a new quad with a new blank node as subject
this._saveContext('blank', this._graph,
this._subject = this._blankNode(), null, null);
this._subject = this._blankNode(undefined, token), null, null);
Copy link
Member

Choose a reason for hiding this comment

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

not undefined

Copy link
Author

Choose a reason for hiding this comment

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

How should I initialize it then? Please advise.

Copy link
Member

Choose a reason for hiding this comment

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

null, or allow a constructor that makes the first argument optional, like this._blankNode({ token }). Then we can do a string test on the first argument.

return this._readBlankNodeHead;
case '(':
// Start a new list
Expand All @@ -206,7 +206,7 @@ export default class N3Parser {
if (!this._n3Mode)
return this._error('Unexpected graph', token);
this._saveContext('formula', this._graph,
this._graph = this._blankNode(), null, null);
this._graph = this._blankNode(undefined, token), null, null);
Copy link
Member

Choose a reason for hiding this comment

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

idem (for all below)

return this._readSubject;
case '}':
// No subject; the graph in which we are reading is closed instead
Expand Down Expand Up @@ -234,7 +234,7 @@ export default class N3Parser {
return this._completeSubjectLiteral;
}
else
this._subject = this._literal(token.value, this._namedNode(token.prefix));
this._subject = this._literal(token.value, this._namedNode(token.prefix, token), token);

break;
case '<<':
Expand Down Expand Up @@ -282,7 +282,7 @@ export default class N3Parser {
if (this._n3Mode) {
// Start a new quad with a new blank node as subject
this._saveContext('blank', this._graph, this._subject,
this._subject = this._blankNode(), null);
this._subject = this._blankNode(undefined, token), null);
return this._readBlankNodeHead;
}
case 'blank':
Expand All @@ -307,12 +307,12 @@ export default class N3Parser {
}
// Pre-datatyped string literal (prefix stores the datatype)
else
this._object = this._literal(token.value, this._namedNode(token.prefix));
this._object = this._literal(token.value, this._namedNode(token.prefix, token), token);
break;
case '[':
// Start a new quad with a new blank node as subject
this._saveContext('blank', this._graph, this._subject, this._predicate,
this._subject = this._blankNode());
this._subject = this._blankNode(undefined, token));
return this._readBlankNodeHead;
case '(':
// Start a new list
Expand All @@ -324,7 +324,7 @@ export default class N3Parser {
if (!this._n3Mode)
return this._error('Unexpected graph', token);
this._saveContext('formula', this._graph, this._subject, this._predicate,
this._graph = this._blankNode());
this._graph = this._blankNode(undefined, token));
return this._readSubject;
case '<<':
if (!this._supportsRDFStar)
Expand Down Expand Up @@ -419,14 +419,14 @@ export default class N3Parser {
case '[':
// Stack the current list quad and start a new quad with a blank node as subject
this._saveContext('blank', this._graph,
list = this._blankNode(), this.RDF_FIRST,
this._subject = item = this._blankNode());
list = this._blankNode(undefined, token), this.RDF_FIRST,
this._subject = item = this._blankNode(undefined, token));
next = this._readBlankNodeHead;
break;
case '(':
// Stack the current list quad and start a new list
this._saveContext('list', this._graph,
list = this._blankNode(), this.RDF_FIRST, this.RDF_NIL);
list = this._blankNode(undefined, token), this.RDF_FIRST, this.RDF_NIL);
this._subject = null;
break;
case ')':
Expand Down Expand Up @@ -462,7 +462,7 @@ export default class N3Parser {
}
// Pre-datatyped string literal (prefix stores the datatype)
else {
item = this._literal(token.value, this._namedNode(token.prefix));
item = this._literal(token.value, this._namedNode(token.prefix, token), token);
next = this._getContextEndReader();
}
break;
Expand All @@ -471,7 +471,7 @@ export default class N3Parser {
if (!this._n3Mode)
return this._error('Unexpected graph', token);
this._saveContext('formula', this._graph, this._subject, this._predicate,
this._graph = this._blankNode());
this._graph = this._blankNode(undefined, token));
return this._readSubject;
default:
if ((item = this._readEntity(token)) === undefined)
Expand All @@ -480,7 +480,7 @@ export default class N3Parser {

// Create a new blank node if no item head was assigned yet
if (list === null)
this._subject = list = this._blankNode();
this._subject = list = this._blankNode(undefined, token);

// Is this the first element of the list?
if (previousList === null) {
Expand Down Expand Up @@ -524,20 +524,20 @@ export default class N3Parser {
// ### `_completeLiteral` completes a literal with an optional datatype or language
_completeLiteral(token) {
// Create a simple string literal by default
let literal = this._literal(this._literalValue);
let literal = this._literal(this._literalValue, undefined, token);

switch (token.type) {
// Create a datatyped literal
case 'type':
case 'typeIRI':
const datatype = this._readEntity(token);
if (datatype === undefined) return; // No datatype means an error occurred
literal = this._literal(this._literalValue, datatype);
literal = this._literal(this._literalValue, datatype, token);
token = null;
break;
// Create a language-tagged string
case 'langcode':
literal = this._literal(this._literalValue, token.value);
literal = this._literal(this._literalValue, token.value, token);
token = null;
break;
}
Expand Down Expand Up @@ -721,7 +721,7 @@ export default class N3Parser {
_readNamedGraphBlankLabel(token) {
if (token.type !== ']')
return this._error('Invalid graph label', token);
this._subject = this._blankNode();
this._subject = this._blankNode(undefined, token);
return this._readGraph;
}

Expand Down Expand Up @@ -751,17 +751,17 @@ export default class N3Parser {
}
// Without explicit quantifiers, map entities to a quantified entity
if (!this._explicitQuantifiers)
this._quantified[entity.id] = this._quantifier(this._blankNode().value);
this._quantified[entity.id] = this._quantifier(this._blankNode(undefined, token).value);
// With explicit quantifiers, output the reified quantifier
else {
// If this is the first item, start a new quantifier list
if (this._subject === null)
this._emit(this._graph || this.DEFAULTGRAPH, this._predicate,
this._subject = this._blankNode(), this.QUANTIFIERS_GRAPH);
this._subject = this._blankNode(undefined, token), this.QUANTIFIERS_GRAPH);
// Otherwise, continue the previous list
else
this._emit(this._subject, this.RDF_REST,
this._subject = this._blankNode(), this.QUANTIFIERS_GRAPH);
this._subject = this._blankNode(undefined, token), this.QUANTIFIERS_GRAPH);
// Output the list item
this._emit(this._subject, this.RDF_FIRST, entity, this.QUANTIFIERS_GRAPH);
}
Expand Down Expand Up @@ -818,7 +818,7 @@ export default class N3Parser {
// ### `_readForwardPath` reads a '!' path
_readForwardPath(token) {
let subject, predicate;
const object = this._blankNode();
const object = this._blankNode(undefined, token);
// The next token is the predicate
if ((predicate = this._readEntity(token)) === undefined)
return;
Expand All @@ -835,7 +835,7 @@ export default class N3Parser {

// ### `_readBackwardPath` reads a '^' path
_readBackwardPath(token) {
const subject = this._blankNode();
const subject = this._blankNode(undefined, token);
let predicate, object;
// The next token is the predicate
if ((predicate = this._readEntity(token)) === undefined)
Expand Down
Loading