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

[faubulous]

I extended the factory methods namedNode, blankNode, literalNodeand variableto also accept an optional token as additional parameter and pass it to the created Term instance.

Then I changed the Parser to provide the tokens as an additional argument when parsing a file.

The default exported DataFactorydoes, however, not pass the token to the factory methods and thus creates nodes without additional added tokens as a default behaviour. This means that the current behaviour is not changed and all existing tests pass.

Using the exported factory methods for one can create a custom DataFactory which can be passed to the parser as an argument using the existing API. This way the exported tokens can either be a) passed as they are or b) modified and stored with the nodes as neded.

[RubenVerborgh]

The more I think about it, it should be:

constructor(whatever, context = DEFAULT_CONTEXT) and this.context = context;

with DEFAULT_CONTEXT a frozen object { token: null } that we recycle.

[faubulous]

Thank you for your quick review @RubenVerborgh! I implemented most of your suggested changes except exporting the DataFactory as is because this will change the default behavior of the API and break some unit tests. Please advise.

[bergos]

I only had a brief look as I am on vacation and traveling, so forgive me if I missed something.

It looks like you are adding arguments to the factory methods, which are defined in the RDF/JS spec. That may cause problems if someone else comes up with the same idea but with different properties. If we want to move forward in that direction, we must extend the specification to avoid any conflicts. It would be good if the spec would cover that topic, but it may take a bit longer and it's understandable if one want to move forward faster.

I would recommend another approach that doesn't require extending the spec. Just pass a flag to the constructor of the parser, and if it's set, set the properties after creating the Term instance with the factory like this:

const term = factory.namedNode(iri)
term.source = { url, line }

Also, please have a look at the additional properties page in the wiki of the spec. I requested creating that page with source maps or DOM element linking in mind. It would be great if we could define a reusable standard structure for it. I have something like this in mind with all properties optional:

term.source = {
  url, // file or http URL
  line, // line number
  position, // start position of the token
  text, // text of the token
  element // DOM element for RDFa
}

I'm back in the week of 22nd of July. It would be great if we could keep the discussion open till the end of that week.

[RubenVerborgh]

@bergos

That may cause problems if someone else comes up with the same idea but with different properties.

Indeed, and thanks for looking at this.

That's the main reason why I am proposing an object as an optional second argument, so { token } rather than directly token or even a string. This should give us the extensibility we're looking for.

Just pass a flag to the constructor of the parser, and if it's set, set the properties after creating the Term instance

I could also agree with this; just not a big fan of external classes managing the internal state of another class. Plus, some factories could create frozen objects etc… I think the first approach, with an optional object, is actually the safest all things considered.

Also, please have a look at the additional properties page in the wiki of the spec. I requested creating that page with source maps or DOM element linking in mind. It would be great if we could define a reusable standard structure for it.

+1 to this

I'm back in the week of 22nd of July. It would be great if we could keep the discussion open till the end of that week.

We'll wait for you, mate. Would be good to have an RDF/JS issue open to track this then. Cheers!