Skip to content

Fixes #2981 #2985

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

Closed
wants to merge 1 commit into from
+251 −208
Closed

Fixes #2981 #2985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/coffee-script/grammar.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions lib/coffee-script/lexer.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

431 changes: 230 additions & 201 deletions lib/coffee-script/parser.js
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions src/grammar.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ grammar =
# The **Root** is the top-level node in the syntax tree. Since we parse bottom-up,
# all parsing must end here.
Root: [
o 'RootContent EOF'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd really love if we could remove parts of this file, instead of adding more...

@michaelficarra @jashkenas Are you fine with EOF being last of each Root items? sigh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was pleasantly surprised by the conciseness of this grammar, given the freedom of the syntax the language provides. But of course, keeping grammar to minimum is always favorable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @Nami-Doc that it's not exactly a good solution. Can we not come up with anything better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what a more concise grammar would look like. Even doing a lot more things ;). Kudos to satyr, as usual.

Copy link
Owner

Choose a reason for hiding this comment

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

@satyr's use of ditto in that file is really silver-tongued.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ugh, the stateful implementation is awful. I would prefer something more declarative. Maybe a block where rules within it have the given action.

ruleBlock = (action, block) -> block action

ruleBlock ((a, b, c) -> some action involving a, b and c), (action) ->
  o 'other rules', action

Copy link
Collaborator

Choose a reason for hiding this comment

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

Congratulations, now you can't read a single thing anymore :/. I don't think that'd improve anything.
Your function is just a do

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I was trying to implement a let, and I forgot we have do which is the almost equivalent.

do (sharedAction = (a, b) -> some action involving a and b) ->
  o 'other rules', sharedAction
  o 'more rules', sharedAction

edit: The point is being explicit rather than implicit. It might hurt the eyes, but it still improves readability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And we need concats because there are arrays, and not every single combination in the block share the same action

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jashkenas what's your stance on this ?

]

# Top-level block of code.
RootContent: [
o '', -> new Block
o 'Body'
o 'Block TERMINATOR'
Expand Down
1 change: 1 addition & 0 deletions src/lexer.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ exports.Lexer = class Lexer

@closeIndentation()
@error "missing #{tag}" if tag = @ends.pop()
@token 'EOF', 'EOF'
return @tokens if opts.rewrite is off
(new Rewriter).rewrite @tokens

Expand Down
4 changes: 2 additions & 2 deletions test/literate.litcoffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,5 @@ and unordered lists, are fine:

Tabs work too:

test "tabbed code", ->
ok yes
test "tabbed code", ->
ok yes
14 changes: 10 additions & 4 deletions test/location.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ x = () ->
test "Verify location of generated tokens", ->
tokens = CoffeeScript.tokens "a = 79"

eq tokens.length, 4
eq tokens.length, 5

aToken = tokens[0]
eq aToken[2].first_line, 0
Expand All @@ -39,10 +39,11 @@ test "Verify location of generated tokens", ->
eq numberToken[2].last_column, 5

test "Verify location of generated tokens (with indented first line)", ->
tokens = CoffeeScript.tokens " a = 83"
tokens = CoffeeScript.tokens " a = 83\nb"

eq tokens.length, 6
[IndentToken, aToken, equalsToken, numberToken] = tokens
eq tokens.length, 9
[indent, aToken, equalsToken, numberToken, outdent,
terminator1, bToken, terminator2, EOF] = tokens

eq aToken[2].first_line, 0
eq aToken[2].first_column, 2
Expand All @@ -59,6 +60,11 @@ test "Verify location of generated tokens (with indented first line)", ->
eq numberToken[2].last_line, 0
eq numberToken[2].last_column, 7

eq bToken[2].first_line, 1
eq bToken[2].first_column, 0
eq bToken[2].last_line, 1
eq bToken[2].last_column, 0

test "Verify all tokens get a location", ->
doesNotThrow ->
tokens = CoffeeScript.tokens testScript
Expand Down