Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add a plugin to provide autoimport functionality - #199

Merged
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport
Nov 3, 2022
Merged

Add a plugin to provide autoimport functionality#199
ccordoba12 merged 45 commits into
python-lsp:developfrom
bagel897:autoimport

Conversation

@bagel897

@bagel897bagel897 commented Apr 18, 2022

Copy link
Copy Markdown
Contributor

Autoimport plugin for pylsp

Depends on accompanying pull request in rope
@bagel897
Closes#34

Prereqs

Rope > 1.0.0 (AKA git builds rn)

Demo Images

image
can detect external modules
image
grabs names from the external modules
image

Features

  • Score-based sorting algorithm with thresholding. Picks the 25 best options.
    Will take suggestions on how to improve the basic algorithm
  • Relatively high preformance using sqllite3 and ast.
    Besides the cache generation, it preforms fine on my machine
  • Avoids suggestions on single line comments, from/normal imports, function/classes, dots
    Most of the test cases are to cover this and it should be pretty functional.
  • Most completion related tests should be in rope
    the main ones are in parsing existing names and whether to suggest imports
  • inserts all imports at the bottom of the set of imports. Recommendation is to use the isort extension.
  • Watches for changes (but not deleted files) and regenerates cache for saved files.

Won't fix

  1. Correct detection of type of statements from __init__.py modules. Would be difficult to preform type inference on.
    image
  2. Doesn't handle deleted files but this is a relatively rare.
  3. Without a progress indicator, the user won't know when its ready (201) I have a WIP implementation based on Add progress reporting #236
  4. Doesn't avoid suggestions on multiple line comments/strings, can fix if deemed important

Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadCONFIGURATION.md Outdated
Other smaller changes:
- Allow memory only database for testing.
- Configuration parameter for memory database
@bagel897

Copy link
Copy Markdown
ContributorAuthor

so this is almost ready:

  1. It needs a fix for windows in rope Fix windows python-rope/rope#477 and associated release
  2. Its still a little rough around where to provide suggestions and slow when it does. Ideally, some of this can be used in jedi and rope completions.
  3. It generates a large cache which takes a while. Normally, this will be on-disk and I will work on improving it. But in CI, this takes a while and must be done seperately for each test and kept in memory. Either I change the scope of the associated fixtures to a module scope, or we have 2m long testing runs. In the future, I'll make it use configured dependencies automatically.

@bagel897bagel897 mentioned this pull request Jun 2, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Some notes about the future of this. I'm working on splitting it out to its own repository where 1-3 things will happen.

  1. Rope-autoimport will use autoimport-core under the hood
  2. Pylsp will use autoimport-core directly
  3. Jedi will use autoimport-core, negating the requirements for any changes in pylsp.

But for now, this is perfectly functional code

@bagel897
bagel897 marked this pull request as ready for review June 22, 2022 23:33
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Yes, but its probably better to mark it off by default, and explain that it is experimental unless it is replaced in jedi/rope

@ccordoba12

Copy link
Copy Markdown
Member

Agreed, that's fine for me too.

@mostley

Copy link
Copy Markdown

Can I help somehow to make this happen?

@bagel897

Copy link
Copy Markdown
ContributorAuthor

Can I help somehow to make this happen?

If Jedi manifested a should_insert API, that'd help - but isn't necessary for the merge. I'm not sure what else is needed.

@ccordoba12ccordoba12 added this to the v1.6.0 milestone Oct 17, 2022
@ccordoba12

Copy link
Copy Markdown
Member

I didn't know this was ready. I'll try to review it this week so we can include it in our next version.

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @bagel897 for your work on this! I left a very lightweight review for you, mostly about style issues. Otherwise looks good to me.

Comment threaddocs/autoimport.md Outdated
Comment threadpylsp/config/schema.json Outdated
Comment threadpylsp/plugins/rope_autoimport.py
Comment threadpylsp/plugins/rope_autoimport.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpylsp/workspace.py Outdated
Comment threadpyproject.toml Outdated
Comment threadtest/plugins/test_autoimport.py
Comment threadtest/plugins/test_autoimport.py Outdated
@ccordoba12ccordoba12 removed this from the v1.6.0 milestone Nov 1, 2022
@bagel897

Copy link
Copy Markdown
ContributorAuthor

Pushed a bugfix and the style changes. Let me know if there's anything else (or if the CI fails, I'll check). You might want to squash the PR, since its a lot of poorly labeled commits, but up to you. Thanks for the feedback @ccordoba12!

@ccordoba12ccordoba12 added this to the v1.7.0 milestone Nov 3, 2022

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing my previous review @bagel897! One last suggestion for you then this should be ready.

Comment threadpylsp/plugins/rope_autoimport.py Outdated
@ccordoba12

Copy link
Copy Markdown
Member

You might want to squash the PR, since its a lot of poorly labeled commits, but up to you

Don't worry about that, we squash merge all PRs in this repo.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

@ccordoba12ccordoba12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bagel897 for this addition! It's a great improvement!!

@ccordoba12ccordoba12 changed the title AutoimportAdd a plugin to provide autoimport functionalityNov 3, 2022
@ccordoba12
ccordoba12 merged commit b24ffd3 into python-lsp:developNov 3, 2022
Sign up for freeto 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.

Auto-import?

5 participants

@bagel897@ccordoba12@wudstrand@mostley@rchl