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

support polymorphism for tags #300

Merged
merged 1 commit into from
Oct 8, 2016
Merged

support polymorphism for tags #300

merged 1 commit into from
Oct 8, 2016

Conversation

monken
Copy link
Contributor

@monken monken commented Sep 24, 2016

This pull requests adds the ability to define multiple kinds per tag. Up to now, every tag could only be of one kind. The tricky part was the handling of null values. When the value is null we will pick one of the type definitions.

Please consider merging this request.

@monken
Copy link
Contributor Author

monken commented Oct 2, 2016

@puzrin any chance to get this merged anytime soon?

@puzrin
Copy link
Member

puzrin commented Oct 2, 2016

Need @dervus to review

@monken
Copy link
Contributor Author

monken commented Oct 8, 2016

There hasn't been any activity in this project sine June. What are the chances that @dervus will have a look at this in the near future? This is kind of a blocker for another project I'm working. I would like to prevent another fork to be released to NPM.

@dervus
Copy link
Collaborator

dervus commented Oct 8, 2016

Sorry I've missed this PR. I have no remarks on the code. There's benchmarks. It doesn't seem this change degrading performance, though I just noticed how we are getting worse results over time. :(

me@here ~/c/js-yaml> node --version
v6.7.0

me@here ~/c/js-yaml> ./benchmark/benchmark.js document
Selected samples: (5 of 14)
 > document_application2
 > document_huge
 > document_nodeca_application
 > document_nodeca_database
 > document_nodeca_logger

Sample: document_application2.yaml (360 characters)
 > current x 46,679 ops/sec ±0.48% (98 runs sampled)
 > js-yaml-3.2.7 x 62,133 ops/sec ±0.58% (94 runs sampled)
 > js-yaml-3.4.6 x 58,813 ops/sec ±0.66% (100 runs sampled)
 > js-yaml-3.6.1 x 45,522 ops/sec ±0.55% (99 runs sampled)


Sample: document_huge.yaml (7182063 characters)
 > current x 4.62 ops/sec ±1.62% (15 runs sampled)
 > js-yaml-3.2.7 x 5.41 ops/sec ±0.71% (17 runs sampled)
 > js-yaml-3.4.6 x 4.82 ops/sec ±0.46% (15 runs sampled)
 > js-yaml-3.6.1 x 4.62 ops/sec ±1.81% (15 runs sampled)


Sample: document_nodeca_application.yaml (2055 characters)
 > current x 37,953 ops/sec ±0.15% (98 runs sampled)
 > js-yaml-3.2.7 x 41,387 ops/sec ±0.12% (101 runs sampled)
 > js-yaml-3.4.6 x 45,096 ops/sec ±0.17% (96 runs sampled)
 > js-yaml-3.6.1 x 38,079 ops/sec ±0.32% (99 runs sampled)


Sample: document_nodeca_database.yaml (394 characters)
 > current x 86,483 ops/sec ±0.76% (92 runs sampled)
 > js-yaml-3.2.7 x 109,252 ops/sec ±0.72% (91 runs sampled)
 > js-yaml-3.4.6 x 107,282 ops/sec ±0.63% (100 runs sampled)
 > js-yaml-3.6.1 x 85,438 ops/sec ±0.77% (90 runs sampled)


Sample: document_nodeca_logger.yaml (727 characters)
 > current x 27,671 ops/sec ±0.20% (97 runs sampled)
 > js-yaml-3.2.7 x 37,012 ops/sec ±0.16% (98 runs sampled)
 > js-yaml-3.4.6 x 34,623 ops/sec ±0.20% (98 runs sampled)
 > js-yaml-3.6.1 x 27,117 ops/sec ±0.29% (102 runs sampled)

@puzrin LGTM

@puzrin puzrin merged commit 2bf232b into nodeca:master Oct 8, 2016
@monken
Copy link
Contributor Author

monken commented Oct 14, 2016

@puzrin thanks for merging. Any chance to get this released anytime soon?

@puzrin
Copy link
Member

puzrin commented Nov 5, 2016

Could anyone clarify about backward compatibility? Will old code continue to work without changes or not?

I mean, if anyone used custom types, does he need to update his code or not?

@monken
Copy link
Contributor Author

monken commented Nov 7, 2016

@puzrin not at all. It's fully backwards compatible. All public and even private API should still function.

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.

3 participants