Skip to content
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.

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

new builder options: tagNameProcessors and attrNameProcessors #462

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jcsahnwaldt
Copy link
Contributor

@jcsahnwaldt jcsahnwaldt commented Jun 9, 2018

  • Added tag and attribute name processing to the builder (similar to what the parser does)
  • Added tests for tag and attribute name processing for the builder (similar to the tests for the parser)
  • Minor refactoring in parser.coffee: processItem was always used in the same way, removed some copied code
  • Added package-lock.json to .gitignore to make git handling simpler. (If you want, we can revert this.)
  • Added a TODO to the line for key, entry of child in builder.coffee - should it be for own key...?
  • Updated README.md.
  • Note: We could also try to add value processors, but I'd rather do that in a separate PR.

Potential issues:

  • Backwards compatibility: Some users may have used the same options object for the parser and the builder in their code. Until now, this worked, because the builder simply ignored tagNameProcessors and attrNameProcessors. After this pull request, such users may run into problems because their builder modifies the names in unexpected ways.
    Possible remedies:
    • Change the names of the builder options, e.g. builderTagNameProcessors and builderAttrNameProcessors.
      • Rather ugly, I think. In the long run, this will be confusing rather than helpful.
    • Add another option that activates tagNameProcessors and attrNameProcessors in the builder, e.g. builderProcessors: true or version: "0.4.20". Old code doesn't contain this option and thus works as usual.
      • Workable, but maybe users will expect all options to be "versioned" in this way...
    • Simply update the documentation and instruct users not to use the same options object for parser and builder. (With object spread syntax (available since Node.js 8.3.0), it's easy to have a common base options object and extend it with different options for builder and parser.)
      • In my opinion, this is the best solution, so I added a few sentences to README.md

@coveralls
Copy link

coveralls commented Jun 9, 2018

Coverage Status

Coverage increased (+0.07%) to 97.708% when pulling b0277eb on iami-me:add_prop_name_processors into 049242d on Leonidas-from-XIV:master.

Also fixed a few minor bugs (normalize -> normalizeTags in one spot, version numbers in attrNameProcessors / attrValueProcessors)
Oops, builder doesn't have value processors (yet).
@MartinLednar
Copy link

Hello guys, when will this be implemented? Thank you 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants