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

Extract new no-single-line-block-comment rule from comment-wrapping rule #1980

Closed
atulgpt opened this issue Apr 26, 2023 · 13 comments · Fixed by #2000
Closed

Extract new no-single-line-block-comment rule from comment-wrapping rule #1980

atulgpt opened this issue Apr 26, 2023 · 13 comments · Fixed by #2000
Milestone

Comments

@atulgpt
Copy link
Contributor

atulgpt commented Apr 26, 2023

Expected Behavior

Comments like /* no-op */ should not be reported by the rule CommentWrapping as in Kotlin these are auto suggested by IDE when typing noop

Observed Behavior

Ktlint reports the /* no-op */ as this is single line comment, ktlint suggests to use // no-op

Your Environment

  • Version of ktlint used: Using Ktlint by detekt at version 0.49.0
@paul-dingemans
Copy link
Collaborator

I can not reproduce this behavior in Intellij. Typing noop does notihing at all. Maybe this behavior is cause by a live template or a plugin. But even if IntelliJ would generate /* no-op */ then I would still not agree to keep it that way. The only acceptable reason would be if /* no-op */ would have a different semantic meaning then // no-op. For example the /* ktlint-disable */ comment can not be replace with // ktlint-disable as the first one starts disabling for a block of lines while the latter only disables the current line.

@paul-dingemans paul-dingemans closed this as not planned Won't fix, can't repro, duplicate, stale Apr 26, 2023
@atulgpt
Copy link
Contributor Author

atulgpt commented Apr 26, 2023

Screenshot 2023-04-27 at 12 53 36 AM

Hi, @paul-dingemans thanks for adding your comments. I have attached the suggestion prompted by the Android studio. Also, this suggestion only comes for the Kotlin file.
This is not configured by any plugin as even with new laptop and new AS install I always got this prompt

@paul-dingemans
Copy link
Collaborator

Ok, cool. Tnx for clarifying that. But still, I do not think it is necessary to keep the block comment if it can be replaced with a EOL comment.

@RBusarow
Copy link

RBusarow commented May 5, 2023

@paul-dingemans could this specific functionality maybe be pulled into a different rule, so that it can be toggled individually?

@paul-dingemans
Copy link
Collaborator

What compelling reason do you have for that? Is Android Studio not working properly anymore after replacing /* no-op */ with // no-op?

@RBusarow
Copy link

RBusarow commented May 5, 2023

I'm not sure that "does the rule break the IDE?" is the right litmus test for this.

The rule does two things. There's nothing inherent in "external content wrapping" which says that block comments should be converted to EOL comments. It simply makes sense to decouple them.

@paul-dingemans
Copy link
Collaborator

The rule does two things. There's nothing inherent in "external content wrapping" which says that block comments should be converted to EOL comments. It simply makes sense to decouple them.

True, that is a valid argument.

@paul-dingemans paul-dingemans reopened this May 6, 2023
@paul-dingemans paul-dingemans changed the title CommentWrapping should ignore /* no-op */ type of comments Extract new no-single-line-block-comment rule from comment-wrapping rule May 6, 2023
@paul-dingemans paul-dingemans added this to the 0.49.1 milestone May 6, 2023
@atulgpt
Copy link
Contributor Author

atulgpt commented May 6, 2023

Hi, @paul-dingemans is this issue up for grabs? (Although I am new to this repo so I might require some guidance as well 😅)

@paul-dingemans
Copy link
Collaborator

Yes sure, please give it a try. But, I want to try to release the 0.49.1 within the next week. If that puts to much stress on you, you might want to pick up something else/

@paul-dingemans
Copy link
Collaborator

@atulgpt
This is the last outstanding issue for the 0.49.1 bugfix release. I will pick it up myself, so that I can start integration testing of the release. Issue #1938 might be a nice once to get started with this code base.

@atulgpt
Copy link
Contributor Author

atulgpt commented May 7, 2023

Hi @paul-dingemans, sure I will pick #1938

@atulgpt
Copy link
Contributor Author

atulgpt commented May 8, 2023

Hi @paul-dingemans in regards to original issues, I still think /* no-op */ should be allowed as // no-op is not valid for all code snippets. For example below code is not a valid one

val foo = { // no-op }

so we must use

 val foo = { /* no-op */ }

in this case. Although in this case, comment wrapping doesn't report anything. But this can create inconsistency throughout the code

@paul-dingemans
Copy link
Collaborator

Hi @paul-dingemans in regards to original issues, I still think /* no-op */ should be allowed as // no-op is not valid for all code snippets. For example below code is not a valid one

val foo = { // no-op }

so we must use

 val foo = { /* no-op */ }

in this case. Although in this case, comment wrapping doesn't report anything. But this can create inconsistency throughout the code

This is already catered for.

    @Test
    fun `Given a single line block containing a block comment then do not reformat`() {
        val code =
            """
            val foo = { /* no-op */ }
            """.trimIndent()
        noSingleLineBlockCommentRuleAssertThat(code)
            .withEditorConfigOverride(CODE_STYLE_PROPERTY to CodeStyleValue.ktlint_official)
            .hasNoLintViolations()
    }

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 a pull request may close this issue.

3 participants