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

Formatter: fixed private constants alignment #4112

Merged

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Mar 4, 2017

Fixed #4111

But I appeared another problem. This file:

private Foo 1
A = 1

is formatted as:

private Foo = 1
A   = 1

I think it is too heavy to fix this problem, and we should not define mixed visibility constants in same segment in general. I will open a new issue for this problem.

@makenowjust
Copy link
Contributor Author

The new issue is here: #4113

@bcardiff
Copy link
Member

bcardiff commented Mar 8, 2017

I agree with @bew 's #4113 (comment)

Let's add a blank line between different visibility constants. I can't think of a reason they should be visually together, and separating them seems to solve the alignment dilema in a trivial way.

@makenowjust if you are willing to implement it, should we hold this PR as in WIP until both #4111 and #4113 are fixed together?

@makenowjust
Copy link
Contributor Author

@bcardiff See #4113 (comment). If segments should be separated by visibility modifiers, I think this PR is independent from #4113 problem, so you can merge this PR without waiting. I'm working to implement @bew's idea now.

Fixed crystal-lang#4111

But now this file

    private Foo 1
    A = 1

is formatted as:

    private Foo = 1
    A   = 1

I think it is too heavy to fix this problem.
@bcardiff bcardiff force-pushed the fix/crystal-format/private-constant branch from 28b5715 to eb1a7cd Compare March 17, 2017 19:00
@bcardiff bcardiff added this to the Next milestone Mar 17, 2017
@bcardiff bcardiff added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Mar 17, 2017
@bcardiff
Copy link
Member

@makenowjust I rebase and apply the formatter to some additional files due to rebase. It should be ok to merge upon CI.

@bcardiff bcardiff merged commit 2065548 into crystal-lang:master Mar 18, 2017
@bcardiff
Copy link
Member

Thanks @makenowjust!

Although it does not apply to the cases handled in this PR, this PR make me think if it is ok to force the format with respect the formatter in master (current behavior) or with the released formatter. The reason is that editor's plugin will use the later and it could be uncomfortable to the user that the saved file will trigger some changes...

@makenowjust makenowjust deleted the fix/crystal-format/private-constant branch March 18, 2017 01:00
ysbaddaden pushed a commit to ysbaddaden/crystal that referenced this pull request Mar 20, 2017
…mat/private-constant

Formatter: fixed private constants alignment
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.

Formatter: private constants alignment does not working
2 participants