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

Fix formatting of lib's fun starting with uppercase letter #5434

Merged
merged 2 commits into from
Dec 22, 2017

Conversation

bew
Copy link
Contributor

@bew bew commented Dec 21, 2017

Fixes #5432

This is my first PR on the formatter 🎉 😃

@bew bew force-pushed the fix-uppercase-fun-formatting branch from f5eb49d to 64a7a3b Compare December 21, 2017 22:42
@@ -581,6 +581,8 @@ describe Crystal::Formatter do
assert_format "lib Foo\nend"
assert_format "lib Foo\ntype Foo = Bar\nend", "lib Foo\n type Foo = Bar\nend"
assert_format "lib Foo\nfun foo\nend", "lib Foo\n fun foo\nend"
assert_format "lib Foo\nfun Bar\nend", "lib Foo\n fun Bar\nend"
assert_format "lib Foo\nfun bar = Bar\nend", "lib Foo\n fun bar = Bar\nend"
Copy link
Contributor

Choose a reason for hiding this comment

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

What about lib Foo\nfun Foo = Bar\nend?

We should probably spec calling functions with CONST names too to make sure that doesn't get broken.

Copy link
Contributor Author

@bew bew Dec 21, 2017

Choose a reason for hiding this comment

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

@RX14 about calling, do you mean add a formatter spec for the lib-uppercase-fun calling?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bew yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@bew bew force-pushed the fix-uppercase-fun-formatting branch from 64a7a3b to 2e395ea Compare December 21, 2017 22:49
write node.name
next_token_skip_space
case @token.type
when :IDENT, :CONST
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to let check receive multiple arguments and check for any of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was thinking about it too, will probably do that

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed anywhere inside formatter.cr? If not, put it in this PR, if it is used in other places, do what those places do and refactor in a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, it looks like it was the only place where a case @token.type was used this way. So I'll leave this change in this PR.

def check(token_type)
raise "expecting #{token_type}, not `#{@token.type}, #{@token.value}`, at #{@token.location}" unless @token.type == token_type
def check(*token_types)
raise "expecting #{token_types.join " or "}, not `#{@token.type}, #{@token.value}`, at #{@token.location}" unless token_types.includes? @token.type
Copy link
Member

Choose a reason for hiding this comment

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

Splitting this from one line is overdue now 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Just change the postfix unless to normal unless and it's readable enough.

@bew bew force-pushed the fix-uppercase-fun-formatting branch from df008b3 to ba7daa9 Compare December 21, 2017 23:08
@RX14 RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:tools:formatter labels Dec 22, 2017
@RX14 RX14 added this to the Next milestone Dec 22, 2017
@bew bew force-pushed the fix-uppercase-fun-formatting branch from befd98b to ca312d1 Compare December 22, 2017 11:44
@bew
Copy link
Contributor Author

bew commented Dec 22, 2017

I squashed the intermediate commits so it should be merge-able as is

@RX14 RX14 merged commit 4932360 into crystal-lang:master Dec 22, 2017
@bew bew deleted the fix-uppercase-fun-formatting branch December 22, 2017 11:57
@straight-shoota
Copy link
Member

Great first formatter contribution! =)

@bew
Copy link
Contributor Author

bew commented Dec 22, 2017

@straight-shoota thanks! I'm glad to have indirectly helped the windows stuff going on these days, by unblocking your pr 😄

@RX14 RX14 modified the milestones: 0.24.1, Next Dec 24, 2017
@matiasgarciaisaia matiasgarciaisaia modified the milestones: Next, 0.24.2 Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:tools:formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants