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

Add code completions to rope_autoimport plugin #471

Merged
merged 21 commits into from
Oct 28, 2023

Conversation

tkrabel-db
Copy link
Contributor

@tkrabel-db tkrabel-db commented Oct 22, 2023

Fixes #403

Testing

  • Unit test
  • Manual test
    image

@tkrabel-db
Copy link
Contributor Author

It's weird. Unrelated tests are failing ...

@krassowski
Copy link
Contributor

Tests are reaching the timeout:

Indeed 7 minutes for pytest here seems suspicious, but there is a small chance it could be GitHub temporarily throttling for no good reason.

@tkrabel-db
Copy link
Contributor Author

@krassowski I see why the test takes so long.

  • In config.py, we expect plugins to have an "enabled" field under their name in order to determine if the plugin is enabled or disabled
  • I remove the top level "enable" from rope_autoimport
  • This makes rope start for every test case, leading to the slow down

In any case, I do think the config should look more like

def pylsp_settings() -> Dict[str, Dict[str, Dict[str, Any]]]:
    # Default rope_completion to disabled
    return {
        "plugins": {
            "rope_autoimport": {
                "enabled": False,  # global "kill switch"
                "memory": False,
                "completions": {
                    "enabled": True, 
                },
                "code_actions": {
                    "enabled": True,
                },
            }
        }
    }

I will address this tomorrow. Need some sleep now :)

@tkrabel-db
Copy link
Contributor Author

Couldn't resist @ccordoba12 @krassowski . I think my PR is ready to review now :)

@tkrabel-db tkrabel-db requested a review from krassowski October 23, 2023 20:59
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @tkrabel-db for your work on this!

pylsp/plugins/rope_autoimport.py Outdated Show resolved Hide resolved
pylsp/plugins/rope_autoimport.py Outdated Show resolved Hide resolved
pylsp/plugins/rope_autoimport.py Show resolved Hide resolved
pylsp/plugins/rope_autoimport.py Show resolved Hide resolved
pylsp/plugins/rope_autoimport.py Show resolved Hide resolved
pylsp/workspace.py Show resolved Hide resolved
test/plugins/test_autoimport.py Outdated Show resolved Hide resolved
# This makes the test below fail because we access the db from a different thread.
# See https://stackoverflow.com/questions/48218065/objects-created-in-a-thread-can-only-be-used-in-that-same-thread
# @pytest.mark.skipif(IS_WIN, reason="Flaky on Windows")
# def test_autoimport_completions_for_notebook_document(
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance to recover this test later? I guess if you left it commented here is because you're planning to do it at some point, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I have an upstream PR open to fix this issue so that we can enable that test later.

@ccordoba12 ccordoba12 changed the title Add code completions to rope_autoimport Add code completions to rope_autoimport plugin Oct 26, 2023
@ccordoba12 ccordoba12 added this to the v1.9.0 milestone Oct 26, 2023
@ccordoba12 ccordoba12 added the enhancement New feature or request label Oct 26, 2023
tkrabel-db and others added 6 commits October 27, 2023 14:07
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
@tkrabel-db tkrabel-db requested a review from ccordoba12 October 27, 2023 12:08
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @tkrabel-db!

@ccordoba12
Copy link
Member

ccordoba12 commented Oct 27, 2023

@krassowski, would you like to give this one another pass? Or should I merge?

Copy link
Contributor

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

There is discussion to be had about how the action kind is handled (e.g. respecting client preferences from initialization request, advertising it in capabilities) but it should not block this PR.

docs/autoimport.md Outdated Show resolved Hide resolved
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
@krassowski krassowski merged commit 2a5a953 into python-lsp:develop Oct 28, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add code action for implementing auto-import
3 participants