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

Handwritten parser #15

Merged
merged 1 commit into from
Jun 15, 2017
Merged

Conversation

motet-a
Copy link
Member

@motet-a motet-a commented Jun 15, 2017

This subjective and opinionated patch fixes many issues but has
several drawbacks.

The parser should be usable in strict mode now (see #9). It is not
longer generated by jison. parse.js contains a simple handwritten
recursive decent parser. It should be easier to debug and to hack, but
there is definitely more code and one can prefer parser generators.
Note that jison seems to have big maintainance issues.

Note that Yargs is not usable in strict mode because it depends on
this package.

I also simplified the lexer (IMHO).

I wrote more Mocha-based tests in test/index.js. I really like the
embedded tests in README.md, but I was afraid to bloat it with
boring spec-compliance tests.

This patch does not introduce a breaking change and does not change
dependencies (however, it changes devDependencies).

@kemitchell
Copy link
Member

@motet-a: Thanks very much for this!

Two quick points:

  1. Do you license this work under The MIT License, as well?

  2. Would you object to squashing the last fix commit into the original rewrite, and opening a separate pull request for the relaxed-mode implementation? I can also do this for you, if you prefer.

@kemitchell kemitchell self-assigned this Jun 15, 2017
@motet-a
Copy link
Member Author

motet-a commented Jun 15, 2017

  1. Of course! Should I add any other licensing information?
  2. I’ll do it right now.

@kemitchell
Copy link
Member

It's late in Lyon! ;-P

@kemitchell
Copy link
Member

No, no more license hassles. We should find an appropriate way to credit you, however.

Thank you for splitting the PR!

@motet-a
Copy link
Member Author

motet-a commented Jun 15, 2017

I have updated the commit message. The tests should pass on older Node.js versions now.

This subjective and opinionated patch fixes many issues but has
several drawbacks.

The parser should be usable in strict mode now (see jslicense#9). It is not
longer generated by `jison`. `parse.js` contains a simple handwritten
recursive decent parser. It should be easier to debug and to hack, but
there is definitely more code and one can prefer parser generators.
Note that `jison` seems to have big maintainance issues.

Note that [Yargs](https://github.com/yargs/yargs) is not usable in strict mode because it depends on
this package.

I also simplified the lexer (IMHO).

I wrote more Mocha-based tests in `test/index.js`. I really like the
embedded tests in `README.md`, but I was afraid to bloat it with
boring spec-compliance tests.

This patch does not introduce a breaking change and does not change
dependencies (however, it changes devDependencies).
@motet-a motet-a force-pushed the opinionated-rewrite branch from b932fc0 to aec7e5d Compare June 15, 2017 21:26
@kemitchell
Copy link
Member

kemitchell commented Jun 15, 2017

@motet-a, I've invited you to the jslicense GitHub org, which in turn gives you write access to this repo. I will make PRs for all non-trivial changes going forward. Please do the same. We can look after each other ... not to mention the code! 😄

As a practical matter, this is now your package as much as it belongs to anyone. Congratulations! It's an important piece of community infrastructure.

That is not my way of saying that I am about to disappear on you. Whether it's dealing with SPDX or npm or other projects, I think there will be plenty more work to do. 😛

I wish I knew how to write this part in French:

This is one of the best rewrites I've seen in open source. It's obvious you took the time to read and understand the spec, and also the existing code. The code is good, and you submitted it in the most respectful and positive way possible. You don't need to hear it from me, but you're good at this.

Technically, I share the opinion that a handwritten parser is best for the task. It was on my list to rewrite in this way for months, but I never found the time. I think you've done it cleaner than I would have.

There are a couple small things to mention:

  1. I've already gone through on my own branch and reverted to ES5 syntax in a few places. On another project, that would be a matter of taste, and I would defer to you. In this package, which goes out with npm and runs on quite a few legacy Node systems, we need to stick to ES5, for maximum compatibility. I think it was just {value} syntax, anyway.

  2. There was some more work to do in package.json to make it ready, which again, I've already done. I think we can do without a separate util module, too, to keep things as small as possible.

If there is anything else, I will bring it up via PRs.

@kemitchell kemitchell merged commit 3f6b178 into jslicense:master Jun 15, 2017
@motet-a
Copy link
Member Author

motet-a commented Jun 16, 2017

Wow, what a long comment! I am really happy to join jslicense. I will try to contribute more to open source projects.

I will make PRs, I’m used to it.

Regarding your questions:

  1. Of course, transpiling with Babel has drawbacks and it’s easier to stick with ES5 in this little project. Since it is used by npm and Yargs, it's important.

  2. Of course, it is nicer without util.js. I wrote it because I thought Array.prototype.map was unavailable on old Node.js versions, and oneOf() was just ridiculous compared to your ||-based replacement 😅

@kemitchell
Copy link
Member

My comment was too long. In short: Welcome!

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.

2 participants