Skip to content

add support for a listener to capture parse events#249

Merged
puzrin merged 1 commit intonodeca:masterfrom
ahgittin:parse_events
Feb 10, 2016
Merged

add support for a listener to capture parse events#249
puzrin merged 1 commit intonodeca:masterfrom
ahgittin:parse_events

Conversation

@ahgittin
Copy link
Copy Markdown
Contributor

This lets a caller build up a parse tree should they need, without impacting anything else.

Inspired by #248 .

@puzrin
Copy link
Copy Markdown
Member

puzrin commented Feb 10, 2016

I don't have principal objection, if you find such hooks useful.

That should not decrease speed, but could you run benchmark for sure? If speed become lower, try to replace if (state.listener) with if (state.listener === null)

/cc @dervus what do you think?

PS. If you executed tests manually, make sure you run linter too (see travis report)

@ahgittin
Copy link
Copy Markdown
Contributor Author

Thanks @puzrin . Delinted. Benchmarks looked very much the same. Seemed slightly better with !== null -- probably negligible and not statistically significant but it made sense so I've done that instead.

@puzrin
Copy link
Copy Markdown
Member

puzrin commented Feb 10, 2016

LGTM. @dervus any comments about option name and so on?

@dervus
Copy link
Copy Markdown
Collaborator

dervus commented Feb 10, 2016

This change seem to imply public exposure of State object, which I don't like because it would make as to maintain backward comparability for that. I don't object if it'll be non-documented and non-supported feature.

puzrin pushed a commit that referenced this pull request Feb 10, 2016
add support for a listener to capture parse events
@puzrin puzrin merged commit 6b86911 into nodeca:master Feb 10, 2016
@puzrin
Copy link
Copy Markdown
Member

puzrin commented Feb 10, 2016

Ok. Let's merge as hidden treasure :)

@puzrin
Copy link
Copy Markdown
Member

puzrin commented Feb 11, 2016

Released with this PR. Enjoy!

ahgittin added a commit to ahgittin/brooklyn-ui that referenced this pull request Feb 11, 2016
from commit 50c82bccf70a19fd4add99542dfdf06a60280da8 (today) with parse listener enhancement nodeca/js-yaml#249
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