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.

Sign up for GitHub

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

Jump to bottom

Convert grammar.pegjs into grammar.pegcoffee #321

Merged
merged 3 commits into from
Aug 23, 2014
Merged

Convert grammar.pegjs into grammar.pegcoffee #321

merged 3 commits into from
Aug 23, 2014

Conversation

lydell
Copy link
Collaborator

@lydell lydell commented Jul 8, 2014

No description provided.

lydell
lydell reviewed Jul 8, 2014
View reviewed changes
src/grammar.pegcoffee
# reduce
while \
stack.length > 2 and \
( \
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be formatted more nicely in coffee-script 1.7, but unfortunately the pegjs-coffee-plugin uses 1.6.

@lydell
Copy link
Collaborator Author

lydell commented Jul 8, 2014

I think this is a good start, but there are some things I don’t really like:

  • I’m not totally sold on that you have to use @var for “global” variables. Might be better to just do it like in plain JavaScript.
  • I common bug that bit me was this: a:Foo? { a ?= []; @rp a}. coffee-script then complains “Error on line 1: the variable "a" can't be assigned with ?= because it has not been declared before”. Rewriting it to a:Foo? { a = [] unless a?; @rp a} doesn’t work either, because that adds var a; at the top of the function, shadowing the “a” label. In such cases I used b:Foo? { a = if b? then b else []; @rp a}. This could be solved by adding all labels as parameters in the added wrapper function, but I’m not sure that is easy to retrieve for plugins.
  • When there are errors in your coffee-script code, the error messages suck. You only get to see all of the cs code that contains the error, and what the error is. No line numbers. This is partly the plugin’s fault (it should use coffee-script 1.7 and expose more), and partly pegjs’s fault (it should add line and column positions to its ast nodes so plugins can base their error locations on them).

Still, this is a step in the right direction. I just don’t feel like forking both the plugin and pegjs to improve them too right now—to tired from this conversion and tricky debugging ....

@lydell
Copy link
Collaborator Author

lydell commented Jul 8, 2014

Looks like the the Travis build fails on Node 0.11—sigh—do you understand why?

@michaelficarra
Copy link
Owner

Thanks, this is great progress.

Looks like the the Travis build fails on Node 0.11—sigh—do you understand why?

Don't worry about it. It's currently broken due to #319

common bug that bit me was [...]

jashkenas/coffeescript interface takes an options object as its second parameter, which may have a locals member (called inScope in CSR because we don't strive for interface parity) that lists names that should be considered already in scope. Ask the plugin maintainer to start using that.

@lydell lydell mentioned this pull request Jul 8, 2014
Closed
@andreineculau
Copy link

A short digression.

I'm not sure what the ultimate goal is with this conversion, but one could also just strip the grammar of actions, and with the help of https://github.com/andreineculau/pegjs-override-action consolidate the actions in a standalone module.

lydell
lydell reviewed Aug 1, 2014
View reviewed changes
src/grammar.pegcoffee
memo
foldr = (fn, memo, list) ->
i = list.length
while i--
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit cabb60a this line was for item in list by -1, which is much nicer, but not yet supported by CSR: #324.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now: 5272d3c.

@lydell
Copy link
Collaborator Author

lydell commented Aug 1, 2014

@michaelficarra could you please have a look at the latest commit. It is a big improvement.

lydell
lydell reviewed Aug 1, 2014
View reviewed changes
Makefile
@@ -30,9 +30,9 @@ lib/bootstrap: lib
mkdir -p lib/bootstrap


lib/parser.js: src/grammar.pegjs bootstraps lib
lib/parser.js: src/grammar.pegcoffee bootstraps lib
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m a bit of a noob regarding Makefiles, but I should add ./lib/pegjs-coffee-plugin.js as a prerequisite here, shouldn’t I?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@lydell
Copy link
Collaborator Author

lydell commented Aug 16, 2014

@michaelficarra Can we try to get this merged? Any change to the grammar is either blocked by this, or requires this PR to be updated.

@lydell
Move from pegjs-coffee-plugin to pegjs-each-code
fd69f35 
- Self-hosted coffee compilation instead of dependency on (old version
  of) jashkenas/coffee-script.
- No more `@variable`s for global variables and helper functions, which
  involves having to use `=>` instead of `->` sometimes. Now, just use
  `variable` like in plain JavaScript.
- It is now possible to do `a:Foo? { a ?= []; rp a}` as expected.
- Better error messages. Partly because CSR has better error messages
  than jashkenas/[email protected], partly because you also get to
  know in which rule the code resides. The error messages will get line
  and column numbers in the .pegcoffee file as soon as
  lydell/pegjs-each-code#1 is resolved.
- No more weird hacks in the initializer.
- Note: lib/parser.js gets pretty bloated because of michaelficarra#323.
lydell added a commit that referenced this pull request Aug 23, 2014
@lydell
Merge pull request #321 from lydell/pegcoffee
a40829b 
Convert grammar.pegjs into grammar.pegcoffee
@lydell lydell merged commit a40829b into michaelficarra:master Aug 23, 2014
@lydell lydell deleted the pegcoffee branch August 23, 2014 15:13
@lydell lydell mentioned this pull request Aug 23, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lydell @michaelficarra @andreineculau