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

Remove comma as enum member delimiter #7618

Merged

Conversation

@j8r
Copy link
Contributor

commented Apr 3, 2019

Resolves #7608

j8r added some commits Apr 3, 2019

@asterite

This comment has been minimized.

Copy link
Member

commented on src/compiler/crystal/tools/formatter.cr in 6704bef Apr 3, 2019

What happens if you remove this if entirely? Just curious, maybe everything still works without this.

This comment has been minimized.

Copy link
Contributor Author

replied Apr 3, 2019

Removing node.name[0].ascii_uppercase? works, but not when removing @token.type == :";". For example:

enum Foo
  A      =   10
  FOO    =  123
  BARBAZ = 1234
end

Formats to

enum Foo
  A      = 10; FOO    = 123; BARBAZ = 1234
end

Instead of

enum Foo
  A      =   10
  FOO    =  123
  BARBAZ = 1234
end

This comment has been minimized.

Copy link
Member

replied Apr 3, 2019

Ah, makes sense

@@ -2060,14 +2060,14 @@ module Crystal
end

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

This comment has been minimized.

Copy link
@oprypin

oprypin Apr 3, 2019

Contributor

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?

This comment has been minimized.

Copy link
@asterite

asterite Apr 3, 2019

Member

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.

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Apr 3, 2019

Member

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 👍

@j8r j8r force-pushed the j8r:remove-comma-as-enum-member-delimiter branch from a145eb5 to 9f6fa91 Apr 3, 2019

@@ -2060,14 +2060,15 @@ module Crystal
end

# This is the case of an enum member
if node.name[0].ascii_uppercase? && @token.type == :","
write ","
# TODO: remove comma support after 0.28.0

This comment has been minimized.

Copy link
@Sija

Sija Apr 3, 2019

Contributor

Support for commas should stay regardless.

This comment has been minimized.

Copy link
@j8r

j8r Apr 3, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Apr 4, 2019

Member

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.

@bcardiff bcardiff added this to the 0.28.0 milestone Apr 12, 2019

@bcardiff bcardiff merged commit d1cdab2 into crystal-lang:master Apr 12, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bcardiff

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Thanks @j8r and reviewers for this addition

@j8r j8r deleted the j8r:remove-comma-as-enum-member-delimiter branch Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.