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

Remove comma as enum member delimiter #7618

Merged
merged 6 commits into from Apr 12, 2019
Merged

Remove comma as enum member delimiter #7618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
307b224
Remove comma as enum member delimiter
j8r Apr 3, 2019
df027d0
Remove enum commas in specs
j8r Apr 3, 2019
6704bef
Remove useless condition
j8r Apr 3, 2019
354c79a
Remove upper case condition
j8r Apr 3, 2019
8a41cfe
Replace comma by semicolon
j8r Apr 3, 2019
9f6fa91
Format comma to semicolon
j8r Apr 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec/compiler/codegen/c_enum_spec.cr
View file
Open in desktop
@@ -1,6 +1,6 @@
require "../../spec_helper"

CodeGenCEnumString = "lib LibFoo; enum Bar; X, Y, Z = 10, W; end end"
CodeGenCEnumString = "lib LibFoo; enum Bar; X; Y; Z = 10; W; end end"

describe "Code gen: c enum" do
it "codegens enum value" do
Expand Down
9 changes: 5 additions & 4 deletions spec/compiler/formatter/formatter_spec.cr
View file
Open in desktop
Expand Up @@ -930,15 +930,16 @@ describe Crystal::Formatter do
assert_format "# Hello\n#\n# ```\n# puts 1+2 # bye\n# 1+2 # hello\n#\n# 1+2\n# ```\n\n# ```\n# puts 1+2\n\n# ```\n# puts 1+2\n\n# Hola\n#\n# 1+2\n# foo do\n# 3+4\n# end\n\n# Hey\n#\n# 1+2\n# foo do\n# 3+4\n# end\n#\n# ```\n# 1+2\n# ```\n#\n# 1+2\n#\n# Bye\n", "# Hello\n#\n# ```\n# puts 1 + 2 # bye\n# 1 + 2 # hello\n#\n# 1 + 2\n# ```\n\n# ```\n# puts 1+2\n\n# ```\n# puts 1+2\n\n# Hola\n#\n# 1+2\n# foo do\n# 3+4\n# end\n\n# Hey\n#\n# 1+2\n# foo do\n# 3+4\n# end\n#\n# ```\n# 1 + 2\n# ```\n#\n# 1+2\n#\n# Bye"
assert_format "macro foo\n {% for value, i in values %}\\\n {% if true %}\\\n {% end %}\\\n {{ 1 }}/\n {% end %}\\\nend\n\n{\n 1 => 2,\n 1234 => 5,\n}\n", "macro foo\n {% for value, i in values %}\\\n {% if true %}\\\n {% end %}\\\n {{ 1 }}/\n {% end %}\\\nend\n\n{\n 1 => 2,\n 1234 => 5,\n}"
assert_format "a = \"\n\"\n1 # 1\n12 # 2\n", "a = \"\n\"\n1 # 1\n12 # 2"
assert_format "enum Foo\n A, B, C\nend\n", "enum Foo\n A, B, C\nend"
assert_format "enum Foo\n A, B, C\nend\n", "enum Foo\n A; B; C\nend"
assert_format "enum Foo\n A; B; C\nend\n", "enum Foo\n A; B; C\nend"
assert_format "# ```\n# macro foo\n# 1\n# end\n# ```\n", "# ```\n# macro foo\n# 1\n# end\n# ```"
assert_format "class Foo\n # ```\n # 1\n # ```\nend\n", "class Foo\n # ```\n # 1\n # ```\nend"
assert_format "# Here is the doc of a method, and contains an example:\n#\n# ```\n# result = foo\n#\n# puts result\n# ```\ndef foo\n # ...\nend\n", "# Here is the doc of a method, and contains an example:\n#\n# ```\n# result = foo\n#\n# puts result\n# ```\ndef foo\n # ...\nend"
assert_format "foo(\n a: 1,\n b: 2,\n )\n", "foo(\n a: 1,\n b: 2,\n)"
assert_format " case 1\n when 2\n 3\n else #:newline, :eof\n 1 if 2\n return 3\n end\n", "case 1\nwhen 2\n 3\nelse # :newline, :eof\n 1 if 2\n return 3\nend"
assert_format "a = 1 if 1 == 2 ||\n 3 == 4\n", "a = 1 if 1 == 2 ||\n 3 == 4"
assert_format "{ A: 1 }\n", "{A: 1}"
assert_format "class Foo\n enum Bar\n A, B, C,\n D, E, F\nend\nend\n", "class Foo\n enum Bar\n A, B, C,\n D, E, F\n end\nend"
assert_format "class Foo\n enum Bar\n A; B; C;\n D; E; F\nend\nend\n", "class Foo\n enum Bar\n A; B; C\n D; E; F\n end\nend"
assert_format "x.is_a? T\n3\n", "x.is_a? T\n3"
assert_format "a = begin\n 1\nend\n\na =\nbegin\n 1\nend\n\na = if 1\n 2\nend\n\nb = 1\nb ||= begin\n 2\nend\n\nb ||= if 1\n 2\nend\n\nb += if 1\n 2\nend\n\nb +=\nif 1\n 2\nend\n\na, b = begin\n 1\nend\n\na, b =\nbegin\n 1\nend\n\nc[x] = begin\n 2\nend\n\nc[x] =\nbegin\n 2\nend\n\nc[x] = if 1\n 2\nend\n\nc[x] ||= begin 1\n 2\nend\n\nc[x] ||= if 1\n 2\nend\n\nc[x] += if 1\n 2\nend\n\nc[x] += begin 1\n 2\nend\n\nc[x] +=\nbegin\n 1\n 2\nend\n\nfoo.bar = begin\nend\n\nfoo.bar =\nbegin\nend\n\nfoo.bar = if\n 2\nend\n\nfoo.bar += begin\n 2\nend\n\nfoo.bar += if\n 2\nend\n\n", "a = begin\n 1\nend\n\na =\n begin\n 1\n end\n\na = if 1\n 2\n end\n\nb = 1\nb ||= begin\n 2\nend\n\nb ||= if 1\n 2\n end\n\nb += if 1\n 2\n end\n\nb +=\n if 1\n 2\n end\n\na, b = begin\n 1\nend\n\na, b =\n begin\n 1\n end\n\nc[x] = begin\n 2\nend\n\nc[x] =\n begin\n 2\n end\n\nc[x] = if 1\n 2\n end\n\nc[x] ||= begin\n 1\n 2\nend\n\nc[x] ||= if 1\n 2\n end\n\nc[x] += if 1\n 2\n end\n\nc[x] += begin\n 1\n 2\nend\n\nc[x] +=\n begin\n 1\n 2\n end\n\nfoo.bar = begin\n\nend\n\nfoo.bar =\n begin\n\n end\n\nfoo.bar = if 2\n end\n\nfoo.bar += begin\n 2\nend\n\nfoo.bar += if 2\n end"
assert_format "module Foo\n 1 # bar\nend\n\nmodule Foo\n 1\n # bar\nend\n\nmodule Foo\n 1\n\n # bar\nend\n\nmodule Foo\n 1\n 2\n # bar\nend\n\nmodule Foo\n 1\n 2\n\n # bar\nend\n\nif 1\n 1\n # bar\nend\n\nif 1\n 1\n\n # bar\nend\n\n1\n2\n# foo\n\n1\n2\n\n# foo\n", "module Foo\n 1 # bar\nend\n\nmodule Foo\n 1\n # bar\nend\n\nmodule Foo\n 1\n\n # bar\nend\n\nmodule Foo\n 1\n 2\n # bar\nend\n\nmodule Foo\n 1\n 2\n\n # bar\nend\n\nif 1\n 1\n # bar\nend\n\nif 1\n 1\n\n # bar\nend\n\n1\n2\n# foo\n\n1\n2\n\n# foo"
Expand Down Expand Up @@ -1326,6 +1327,6 @@ describe Crystal::Formatter do
assert_format "def foo(x) : Int32 # bar\n # baz\nend"
assert_format "def foo(x) forall T # bar\n # baz\nend"

# #7600
assert_format "enum E\n A, # hello\n B, # hello\n C, # hello\nend"
# #7608
assert_format "enum E\n A # hello\n B # hello; C # hello\nend"
end
10 changes: 5 additions & 5 deletions spec/compiler/parser/parser_spec.cr
View file
Open in desktop
Expand Up @@ -779,8 +779,8 @@ module Crystal
it_parses "lib LibC; struct Foo; x : Int**; end end", LibDef.new("LibC", [CStructOrUnionDef.new("Foo", Expressions.from(TypeDeclaration.new("x".var, "Int".path.pointer_of.pointer_of)))] of ASTNode)
it_parses "lib LibC; struct Foo; x, y, z : Int; end end", LibDef.new("LibC", [CStructOrUnionDef.new("Foo", [TypeDeclaration.new("x".var, "Int".path), TypeDeclaration.new("y".var, "Int".path), TypeDeclaration.new("z".var, "Int".path)] of ASTNode)] of ASTNode)
it_parses "lib LibC; union Foo; end end", LibDef.new("LibC", [CStructOrUnionDef.new("Foo", union: true)] of ASTNode)
it_parses "lib LibC; enum Foo; A\nB, C\nD = 1; end end", LibDef.new("LibC", [EnumDef.new("Foo".path, [Arg.new("A"), Arg.new("B"), Arg.new("C"), Arg.new("D", 1.int32)] of ASTNode)] of ASTNode)
it_parses "lib LibC; enum Foo; A = 1, B; end end", LibDef.new("LibC", [EnumDef.new("Foo".path, [Arg.new("A", 1.int32), Arg.new("B")] of ASTNode)] of ASTNode)
it_parses "lib LibC; enum Foo; A\nB; C\nD = 1; end end", LibDef.new("LibC", [EnumDef.new("Foo".path, [Arg.new("A"), Arg.new("B"), Arg.new("C"), Arg.new("D", 1.int32)] of ASTNode)] of ASTNode)
it_parses "lib LibC; enum Foo; A = 1; B; end end", LibDef.new("LibC", [EnumDef.new("Foo".path, [Arg.new("A", 1.int32), Arg.new("B")] of ASTNode)] of ASTNode)
it_parses "lib LibC; Foo = 1; end", LibDef.new("LibC", [Assign.new("Foo".path, 1.int32)] of ASTNode)
it_parses "lib LibC\nfun getch = GetChar\nend", LibDef.new("LibC", [FunDef.new("getch", real_name: "GetChar")] of ASTNode)
it_parses %(lib LibC\nfun getch = "get.char"\nend), LibDef.new("LibC", [FunDef.new("getch", real_name: "get.char")] of ASTNode)
Expand Down Expand Up @@ -1342,8 +1342,8 @@ module Crystal
it_parses "x, y, z = <<-FOO, <<-BAR, <<-BAZ\nhello\nFOO\nworld\nBAR\n!\nBAZ",
MultiAssign.new(["x".var, "y".var, "z".var] of ASTNode, ["hello".string_interpolation, "world".string_interpolation, "!".string_interpolation] of ASTNode)

it_parses "enum Foo; A\nB, C\nD = 1; end", EnumDef.new("Foo".path, [Arg.new("A"), Arg.new("B"), Arg.new("C"), Arg.new("D", 1.int32)] of ASTNode)
it_parses "enum Foo; A = 1, B; end", EnumDef.new("Foo".path, [Arg.new("A", 1.int32), Arg.new("B")] of ASTNode)
it_parses "enum Foo; A\nB; C\nD = 1; end", EnumDef.new("Foo".path, [Arg.new("A"), Arg.new("B"), Arg.new("C"), Arg.new("D", 1.int32)] of ASTNode)
it_parses "enum Foo; A = 1; B; end", EnumDef.new("Foo".path, [Arg.new("A", 1.int32), Arg.new("B")] of ASTNode)
it_parses "enum Foo : UInt16; end", EnumDef.new("Foo".path, base_type: "UInt16".path)
it_parses "enum Foo; def foo; 1; end; end", EnumDef.new("Foo".path, [Def.new("foo", body: 1.int32)] of ASTNode)
it_parses "enum Foo; A = 1\ndef foo; 1; end; end", EnumDef.new("Foo".path, [Arg.new("A", 1.int32), Def.new("foo", body: 1.int32)] of ASTNode)
Expand All @@ -1363,7 +1363,7 @@ module Crystal

it_parses "enum Foo; @[Bar]; end", EnumDef.new("Foo".path, [Annotation.new("Bar".path)] of ASTNode)

assert_syntax_error "enum Foo; A B; end", "expecting ',', ';', 'end' or newline after enum member"
assert_syntax_error "enum Foo; A B; end", "expecting ';', 'end' or newline after enum member"

it_parses "1.[](2)", Call.new(1.int32, "[]", 2.int32)
it_parses "1.[]?(2)", Call.new(1.int32, "[]?", 2.int32)
Expand Down
2 changes: 1 addition & 1 deletion spec/compiler/semantic/c_enum_spec.cr
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ require "../../spec_helper"

describe "Semantic: c enum" do
it "types enum value" do
assert_type("lib LibFoo; enum Bar; X, Y, Z = 10, W; end; end; LibFoo::Bar::X") { types["LibFoo"].types["Bar"] }
assert_type("lib LibFoo; enum Bar; X; Y; Z = 10, W; end; end; LibFoo::Bar::X") { types["LibFoo"].types["Bar"] }
end

it "allows using an enum as a type in a fun" do
Expand Down
4 changes: 3 additions & 1 deletion spec/compiler/semantic/instance_var_spec.cr
View file
Open in desktop
Expand Up @@ -1247,7 +1247,9 @@ describe "Semantic: instance var" do
it "infers type from enum member" do
assert_type(%(
enum Color
Red, Green, Blue
Red
Green
Blue
end

class Foo
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/crystal/syntax/parser.cr
View file
Open in desktop
Expand Up @@ -5560,11 +5560,12 @@ module Crystal
skip_space

case @token.type
# TODO: remove comma support after 0.28.0
when :",", :";", :NEWLINE, :EOF
next_token_skip_statement_end
else
unless @token.keyword?(:end)
raise "expecting ',', ';', 'end' or newline after enum member", location
raise "expecting ';', 'end' or newline after enum member", location
end
end

Expand Down
5 changes: 3 additions & 2 deletions src/compiler/crystal/tools/formatter.cr
View file
Open in desktop
Expand Up @@ -2060,14 +2060,15 @@ module Crystal
end

# This is the case of an enum member
if node.name[0].ascii_uppercase? && @token.type == :","
write ","
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the usual policy for the formatter is, but might it be possible to tell it to chew up old code with commas and re-format it, rather than outright failing?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea! We've done it in the past. But for that the parser will still have to support commas, or at least the parser used in the formatter (could be toggled with a flag). I like that idea.

Copy link
Member

Choose a reason for hiding this comment

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

That would be similar to an autocorrection feature for check, but implemented in the formatter.
I don't think it's urgent to completely abolish commas right away, so formatting them to semicolons seems like a good idea 👍

# TODO: remove comma support after 0.28.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Support for commas should stay regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the consensus from #7608, though no dead line is decided. It could be pushed further.

Copy link
Member

Choose a reason for hiding this comment

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

Having this TODO is certainly better than not, even if we later decided to keep this conversion indefinitely. But I don't think that's reasonable. We don't need to throw it out in the next version, but eventually it would just be useless dead code.

if @token.type == :";" || (node.name[0].ascii_uppercase? && @token.type == :",")
next_token
@lexer.skip_space
if @token.type == :COMMENT
write_comment
@exp_needs_indent = true
else
write ";" if @token.type == :CONST
write " "
@exp_needs_indent = @token.type == :NEWLINE
end
Expand Down