feat(common): add utility functions for xmlns attributes #173
Conversation
|
|
packages/common/lib/xml-ns-key.js
Outdated
| if (matchArr === null) { | ||
| return false; | ||
| } | ||
| if (includeEmptyPrefix === true) { |
There was a problem hiding this comment.
if (includeEmptyPrefix === true) { correctness depends on running after:
if (matchArr === null) { return false; }
should an else if be used to explicitly mark this relationship?
There was a problem hiding this comment.
I prefer not to, would it be enough to add a comment?
packages/common/lib/xml-ns-key.js
Outdated
| return true; | ||
| } | ||
| /* istanbul ignore next - defensive coding */ | ||
| const groups = matchArr.groups || {}; |
There was a problem hiding this comment.
I don't think we need the || {} in JavaScript.
Because we know that in Node 10.0+ named capture groups are supported and that
the match succeed if we reached this line.
In TypeScript we used || {} to calm to compiler as the .groups property is defined as optional. so its not really defensive coding, we can prove it can't reach this branch so we don't actually need the branch.
There was a problem hiding this comment.
ok, I'll remove it
packages/common/lib/xml-ns-key.js
Outdated
| /* istanbul ignore next - defensive coding */ | ||
| const groups = matchArr.groups || {}; | ||
| // In the case where key === "xmlns:", prefix will be empty and prefixWithColon will not be empty | ||
| return !(groups.prefixWithColon && !groups.prefix); |
There was a problem hiding this comment.
Would it be simpler to have more distinct groups in the regExp? (no overlap)
- xmlns
- colon
- prefix
and then ask if 1 &2 exists (empty prefix case) or 1 & 2 & 3 exists (proper xmlns)
There was a problem hiding this comment.
I changed the condition a bit to make it clearer, let me know if you think I should still change this
packages/common/api.d.ts
Outdated
| * @param key - the attribute key | ||
| * @param includeEmptyPrefix - should true be returned when there is no prefix (key === "xmlns:") | ||
| */ | ||
| export function isXMLNamespaceKey( |
There was a problem hiding this comment.
should the term prefix be part of this name somehow?
I think that is the term used in the XML specs, perhaps instead of the key which is related to the attribute but we are dealing with a string here. (more like a tokenizer)
There was a problem hiding this comment.
The function is only meaningful when related to xml attribute keys, a regular string isn't a namespace key, I wanted to make it clear
packages/common/lib/xml-ns-key.js
Outdated
| // xmlns attributes explicitly can't contain ":" after the "xmlns:" part. | ||
| const namespaceRegex = /^xmlns(?<prefixWithColon>:(?<prefix>[^:]*))?$/; | ||
|
|
||
| function isXMLNamespaceKey(key, includeEmptyPrefix) { |
There was a problem hiding this comment.
use simple JSDocs to give IDE hints for types: e.g:
There was a problem hiding this comment.
The link doesn't work
There was a problem hiding this comment.
No description provided.