Skip to content

Rewrite to no-indent keeps end comment #12954

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

Merged
merged 2 commits into from
Jul 8, 2021
Merged

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jun 27, 2021

Preserve end marker as end comment, since if it was worth having an end marker, it's worth commenting.

class C:
  def f = 42
end C

is rewritten to

class C {
  def f = 42
} // end C

@som-snytt som-snytt marked this pull request as ready for review June 30, 2021 04:27
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM

@som-snytt
Copy link
Contributor Author

Deprecating "empty colonized template bodies" may be undesirable because accepting it was intentional; however, it is an excuse to use the phrase "empty colonized template bodies" outside a sci-fi context or academic paper.

@dwijnand
Copy link
Member

dwijnand commented Jul 7, 2021

Could you propose that separately then? Personally I don't like it, but I also don't like that class C: is bad syntax too - and changing that would make your consistency point.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

^

@som-snytt som-snytt marked this pull request as draft July 7, 2021 16:54
@som-snytt
Copy link
Contributor Author

@dwijnand Rebased and kept only the feature described.

I'll propose deprecating empty body after colon separately; I don't feel strongly about it, except that "end marker is optional" strikes me as a useful rule that should not be violated lightly. (Note to self for later.)

Also dropped converting the converse, end comment to end marker, because it was just a heuristic that did not verify the identifier. Probably auto-end-marking is a more useful and consistent feature, for which there is a ticket; maybe the conversion can be done then.

@som-snytt som-snytt marked this pull request as ready for review July 7, 2021 19:28
@bishabosha bishabosha requested a review from dwijnand July 8, 2021 12:46
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Som

@dwijnand dwijnand merged commit 110e5cb into scala:master Jul 8, 2021
@som-snytt som-snytt deleted the issue/12340 branch July 8, 2021 15:21
Comment on lines +23 to +26
pbuf.indexWhere(p => p.span.start == span.start && p.span.end == span.end) match {
case i if i >= 0 => pbuf.update(i, Patch(span, replacement))
case _ => pbuf += Patch(span, replacement)
}
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this to be the root cause of #17187

object A:
    object B:
        def a = 2

When rewriting to no-indent, we want to add }\n and then }\n at the exact same position. But this piece of code only keep the second patch, which leads to the incorrect:

object A {
    object B {
        def a = 2
}

@adpi2
Copy link
Member

adpi2 commented Apr 28, 2023

Tested also this:

class C:
  def f = 42

end C

And it is rewritten to this:

class C {
  def f = 42
}

end C

@som-snytt
Copy link
Contributor Author

@adpi2 it looks like it introduced "replace" semantics for "patch at a point". I don't remember now why. There is a tweak to "overlaps" to include overlaps if exactly one point.

ckipp01 added a commit to ckipp01/dotty that referenced this pull request May 4, 2023
This adds in a test to ensure that the issue reported in scala#17399 is
indeed fixed with scala#12954. This case is a bit different, so best to also
just have it covered with a test.

Closes scala#17399
ckipp01 added a commit that referenced this pull request May 4, 2023
This adds in a test to ensure that the issue reported in #17399 is
indeed fixed with #12954. This case is a bit different, so best to also
just have it covered with a test.

Closes #17399
@Kordyjan Kordyjan added this to the 3.0.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants