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

Format code in comments following # => #8851

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fschrfl
Copy link

@fschrfl fschrfl commented Feb 25, 2020

Implements the feature discussed in #7272.

Using the same example as in the issue:

a = [1, 2, 3]
a.permutations    # => [[1,2,3],[1,3,2],[2,1,3],[2,3,1],[3,1,2],[3,2,1]]
a.permutations(1) # => [[1],[2],[3]]
a.permutations(2) # => [[1,2],[1,3],[2,1],[2,3],[3,1],[3,2]]
a.permutations(3) # => [[1,2,3],[1,3,2],[2,1,3],[2,3,1],[3,1,2],[3,2,1]]
a.permutations(0) # => [[]]
a.permutations(4) # => []

The above is now formatted like this:

a = [1, 2, 3]
a.permutations    # => [[1, 2, 3], [1, 3, 2], [2, 1, 3], [2, 3, 1], [3, 1, 2], [3, 2, 1]]
a.permutations(1) # => [[1], [2], [3]]
a.permutations(2) # => [[1, 2], [1, 3], [2, 1], [2, 3], [3, 1], [3, 2]]
a.permutations(3) # => [[1, 2, 3], [1, 3, 2], [2, 1, 3], [2, 3, 1], [3, 1, 2], [3, 2, 1]]
a.permutations(0) # => [[]]
a.permutations(4) # => []

Note that the last two lines are no valid Crystal expressions in which case the formatter doesn't change them.

This is my first attempt at a contribution to the project so if I did anything wrong please let me know and I will try to improve.

Copy link
Contributor

@j8r j8r left a comment

Choose a reason for hiding this comment

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

Tests are missing

src/compiler/crystal/tools/formatter.cr Outdated Show resolved Hide resolved
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Looks great! Awesome first contribution ❤️

As @j8r said, having a few specs would be nice (see spec/compiler/formatter/formatter_spec.cr).

Comment on lines 4518 to 4521
if after_comment_value.starts_with?("=>")
value = "\# => #{after_comment_value[2..-1].strip}"
# If a comment starts with `=>`, it indicates the result of an expression which
# is a valid expression itself and should be properly formatted.
formatted_result_expression = after_comment_value[2..-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be replaced by:
if formatted_result_expression = after_comment_value.lchop? "=>"

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, both the initial if condition and the line with [2..-1] can be replaced by the above expression.

@fschrfl
Copy link
Author

fschrfl commented Feb 25, 2020

I added some specs and adjusted the code from the first commit based on your suggestions. Thanks for the feedback. Hope, the specs cover everything. I am not very imaginative when it comes to test.

spec/compiler/formatter/formatter_spec.cr Outdated Show resolved Hide resolved
spec/compiler/formatter/formatter_spec.cr Outdated Show resolved Hide resolved
src/compiler/crystal/tools/formatter.cr Outdated Show resolved Hide resolved

# format expressions in `# =>` comments
assert_format "1 # => ?invalid code"
assert_format "1 # => #=>1+2", "1 # => # => 1 + 2"
Copy link
Member

Choose a reason for hiding this comment

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

These specs cover known edge cases but it's as important to test a few basic cases first.

Copy link
Author

Choose a reason for hiding this comment

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

What basic cases would you suggest? The two cases that need to be covered are invalid and valid code. Invalid code is covered by the first case, for valid code I could add another basic case like 1 # => 1 + 2 but that is also already covered by the recursion in case two.

Copy link
Member

Choose a reason for hiding this comment

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

What. happens if the comment expression isa method definition with, say, 1 + 2 as the body?

Copy link
Author

Choose a reason for hiding this comment

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

That is indeed a problem I did not consider. As it is now, the formatter will put the body and end in separate lines which are not commented out:

1 # => def add12() 1 + 2 end

turns into

1 # => def add12
  1 + 2
end

It would be a good idea, anyway, to only apply formatting to certain kinds of expressions, or better still, to only apply formatting if the formatted string is equal to the result of .inspecting the expression before the comment (see discussion in #7272). If that was implemented, the above problem wouldn't even come up. So before fixing this, it might be a good idea to overthink the whole approach here...

Copy link
Member

Choose a reason for hiding this comment

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

I think it's simpler: try to format it. If you find newlines, then don't use the result.

Copy link
Member

Choose a reason for hiding this comment

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

Specially considering that it's a tool that's combined with the editor. I would find it pretty annoying if the editor starts spitting errors at me because of the way I organize my code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe the formatter is not the right tool for this. But it would be great to have some formal validation for # => comments and similar.
Maybe that's something for ameba?

Copy link
Member

Choose a reason for hiding this comment

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

If anything, a tool like doctest would be great.

I think having the formatter try to format the value if possible is good enough. Then doctest would actually run it and verify that the results are what you'd expect them to be.

Copy link
Author

@fschrfl fschrfl Mar 2, 2020

Choose a reason for hiding this comment

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

Incidentally, is there a reason why def add12 : Int32 1 + 2 end and def add12() 1 + 2 end are valid syntax while def add12 1 + 2 end raises the syntax error "unexpected token: 1"?

Copy link
Member

Choose a reason for hiding this comment

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

Just a lack of thought from us on those details. We should probably copy Ruby or decide what to do, but it's very low priority.

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

I'd like to wait until #7272 is discussed a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants