API for testing the type of a node

[tmpfs]

Updating to use the recent changes we discussed I got bitten by the fact that I was using magic strings for comparing against Node.type, I suggest that Node expose constants for each type so that this can be avoided in the future and allows changing the type naming convention without impacting consumers of the AST.

I am happy to do the leg work, check tests/benchmarks and submit a PR if you agree this is a good move.

[jgm]

What do you mean by "magic strings"?

+++ muji [Mar 15 16 04:17 ]:

Updating to use the recent changes we discussed I got bitten by the fact that I was using magic strings for comparing against Node.type, I suggest that Node expose constants for each type so that this can be avoided in the future and allows changing the type naming convention without impacting consumers of the AST.

I am happy to do the leg work, check tests/benchmarks and submit a PR if you agree this is a good move.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: [1]https://github.com/jgm/commonmark.js/issues/92

References

1. https://github.com/jgm/commonmark.js/issues/92

[tmpfs]

The term magic strings is used sometimes to refer to string literals in code that would be better represented via a variable or constant as they are fragile due to the potential for change elsewhere.

Let's say I want to know if a node is a code block, I do:

if(node.type === 'code_block') {

}

But if you decided to change the format for type strings (say back to CodeBlock) my code is now badly broken.

However if I could compare against a constant:

if(node.type === Node.CODE_BLOCK) {

}

Then a whole class of potential problems goes away

Hope that clarifies a bit.

[tmpfs]

For a sketch of what I am thinking should be exposed by Node I am implementing a subclass here:

https://github.com/mkdoc/mkast/blob/master/lib/node.js

[tmpfs]

The createDocument function was lifted from lib/blocks.js and could be moved to Node so that AST consumers can also access it (blocks.js does not export it).

[tmpfs]

With the is() method the API for testing node type then becomes:

if(node.is(Node.CODE_BLOCK)) {}

Which would completely abstract away the underlying type identifier from AST consumers, you could switch to using integers as type identifiers for example and nobody would be affected provided they tested node type as above.

[jgm]

I really hate programming in languages without the ability to create arbitrary sum types!

Anyway, since we're stuck with one, you're probably right that using a constant would be better. It would be good to measure if this had a performance cost, though, using existing benchmarks. Making the values integers, and providing a further function to return a string label (which can be used in the xml writer and elsewhere), might also make sense, but only if it speeds things up noticeably.

[tmpfs]

Ha! Sum types were new to me, but I like the look of them :)

Instead we can use bitwise OR using integer identifiers. And then the API for testing node type would look something like:

Node.is(node, Node.PARAGRAPH | Node.CODE_BLOCK);

I imagine that a) it will be a fairly big refactor to integer identifiers (exposing the integer to string method is trivial of course) and b) there won't be much in terms of speed up, although I fail to see how string comparison could ever be faster than integer comparison (I don't think it will slow down).

Note that I am leaning towards a static is function now as I am serializing and deserializing nodes so often I have an object that isn't a Node but would like to test for type on.