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

TYPE: implement SelfManagingCommenter #5416

Merged
merged 6 commits into from May 27, 2020
Merged

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented May 21, 2020

This PR solves #5388 with a pretty heavyweight solution of implementing SelfManagingCommenter. The implementation is pretty basic, but it passes all the original RsCommenter tests and some new ones that I have added.

It seems to work for basic use cases, if you can find some counter-examples where it breaks, let me know and I'll add them to the test suite.

Fixes: #5388

@mchernyavsky mchernyavsky added this to In Progress in To test via automation May 21, 2020
Copy link
Member

@vlad20012 vlad20012 left a comment

Choose a reason for hiding this comment

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

👍 It looks much simpler than I expected! Thanks!

src/main/kotlin/org/rust/ide/commenter/RsCommenter.kt Outdated Show resolved Hide resolved
src/main/kotlin/org/rust/ide/commenter/RsCommenter.kt Outdated Show resolved Hide resolved
src/main/kotlin/org/rust/ide/commenter/RsCommenter.kt Outdated Show resolved Hide resolved
src/main/kotlin/org/rust/ide/commenter/RsCommenter.kt Outdated Show resolved Hide resolved
@Kobzol
Copy link
Member Author

Kobzol commented May 23, 2020

Added test for space after line comment settings and fixed the rest. Should getCommentPrefix respect the space after line comment settings and return // if it's turned on? It seems to work if I just return // everytime.

There's something weird with the test move caret after commenting empty line test. Instead of

//<caret>
fn foo() {}

it returns

//
<caret>fn foo() {}

even though when I try it in the IDE, it behaves correctly.

@vlad20012
Copy link
Member

vlad20012 commented May 25, 2020

Should getCommentPrefix respect the space after line comment settings and return // if it's turned on?

It looks like no, simple // is enough

There's something weird with the test move caret after commenting empty line test

This was tricky, but I found the reason. There is a mutable state in ActionManagerImpl that outlives a test. It's info about previous run action - myPrevPerformedActionId. Then it's used in CommentByLineCommentHandler.invoke - see startingNewLineComment condition. I don't know is it a bug or a feature of the test framework.

I've changed the code of checkOption like this and looks like it works as expected now:

private fun checkOption(optionProperty: KMutableProperty0<Boolean>,
                            @Language("Rust") before: String,
                            @Language("Rust") afterOn: String,
                            @Language("Rust") afterOff: String,
                            actionId: String,
                            trimIndent: Boolean = true) {
        val initialValue = optionProperty.get()
        optionProperty.set(true)
        try {
            checkEditorAction(before, afterOn, actionId, trimIndent = trimIndent)
            resetActionManagerState()
            optionProperty.set(false)
            checkEditorAction(before, afterOff, actionId, trimIndent = trimIndent)
            resetActionManagerState()
        } finally {
            optionProperty.set(initialValue)
        }
    }

    private fun settings() = CodeStyle.getSettings(project).getCommonSettings(RsLanguage)

    /**
     * Resets [com.intellij.openapi.actionSystem.impl.ActionManagerImpl.myPrevPerformedActionId].
     * Otherwise it affect [com.intellij.codeInsight.generation.CommentByLineCommentHandler.invoke]
     * (see `startingNewLineComment` condition there).
     */
    private fun resetActionManagerState() {
        myFixture.performEditorAction(EMPTY_ACTION_ID)
    }

    override fun setUp() {
        super.setUp()
        ActionManager.getInstance().registerAction(EMPTY_ACTION_ID, EmptyAction.createEmptyAction("empty", null, true))
    }

    override fun tearDown() {
        try {
            super.tearDown()
        } finally {
            ActionManager.getInstance().unregisterAction(EMPTY_ACTION_ID)
        }
    }

    companion object {
        private const val EMPTY_ACTION_ID = "!!!EmptyAction"
    }

@Kobzol
Copy link
Member Author

Kobzol commented May 25, 2020

Thank you! It works locally, but it seems that CI is not happy for some reason..

@vlad20012
Copy link
Member

Hmm
bors try

bors bot added a commit that referenced this pull request May 25, 2020
@bors
Copy link
Contributor

bors bot commented May 25, 2020

try

Build failed:

@vlad20012
Copy link
Member

Let's also add this:

override fun checkEditorAction(before: String, after: String, actionId: String, trimIndent: Boolean, testmark: Testmark?) {
        super.checkEditorAction(before, after, actionId, trimIndent, testmark)
        resetActionManagerState()

    }

Looks like it fails on CI because of different test order when tests are run by Gradle

@vlad20012
Copy link
Member

👍
bors r+

@vlad20012 vlad20012 added this to the v124 milestone May 27, 2020
@bors
Copy link
Contributor

bors bot commented May 27, 2020

@bors bors bot merged commit aac5ff2 into intellij-rust:master May 27, 2020
To test automation moved this from In Progress to Test May 27, 2020
@lancelote lancelote moved this from Test to Done in To test Jun 2, 2020
@Kobzol Kobzol deleted the commenter branch June 3, 2020 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
To test
  
Done
Development

Successfully merging this pull request may close these issues.

Uncommenting a doc comment
4 participants