Autoimport autodetection #516
Conversation
|
@bagel897 other than Database versioning, is there any other things that this PR will need to resolve before this draft PR is "ready for review"? |
|
I'm going to be very sporadic on all PRs, sorry. |
|
No worries, if you are not already working on it, I might pick the task for that database schema versioning so the DB versioning probably should have its own ticket. |
|
I'll move the remaining stuff to future PRs |
|
@lieryan Sorry for the miscommunication - this PR should be ready. |
bagel897
force-pushed
the
autoimport_autodetect
branch
from
5354582 to
20170be
Compare
bagel897
force-pushed
the
autoimport_autodetect
branch
from
20170be to
310fafe
Compare
|
@lieryan I finally got this one ready, thoughts? |
rope/contrib/autoimport/sqlite.py
Outdated
| project_package = get_package_tuple(project.root.pathlib, project) | ||
| assert project_package is not None | ||
| assert project_package.path is not None | ||
| if underlined is not None: | ||
| self.prefs.underlined = underlined |
There was a problem hiding this comment.
Don't we need to copy the self.prefs here if we're going to mutate it? Otherwise it would change the setting globally and in other instances of AutoImport that shares the same prefs.
There was a problem hiding this comment.
Good point - I ran a deep copy. Maybe in the future, it should automatically reload on pyproject.toml changes? Would you want me to include that in this PR?
rope/base/prefs.py
Outdated
| underlined: bool = field( | ||
| default=False, description="Cache underlined (private) modules") | ||
| memory: bool = field(default=None, description="Cache in memory instead of disk") | ||
| parallel: bool = field(default=True, description="Use multiple processes to parse") |
There was a problem hiding this comment.
Thinking about this further on the config. A couple thoughts:
-
I don't think
parallelshould be user-configurable. I'd have preferred to not make parallelization configurable at all, but if there's use case single-threaded mode, I think it's generally the editor/IDE integrators that would need to choose the best value for their integration, not the user. It's generally going to be a matter of whether their environment supports/allows multithreading.- Maybe we can make some of these hidden/advanced settings, or add warnings that users generally shouldn't be toggling them unless directed by a rope/integration developer
-
I'm not quite sure whether
memoryshould be global or autoimport-specific configuration. While this setting currently only affects autoimport, we may want to expand this to other caches. Users that don't want rope to create extra files in the project directory likely would want to just turn off all cached files and users that are fine with.ropeprojectfolder mostly wouldn't care about the different cache files that rope generates.- It's only a small amount of very advanced users or users that are having very specific problems that would've cared to vary the
memoryper internal rope component. Unless there's a strong use case for per-component fine tuning here, I would rather keep things that can be configured to a minimum to keep maintenance simpler.
- It's only a small amount of very advanced users or users that are having very specific problems that would've cared to vary the
docs/configuration.rst
Outdated
| @@ -14,6 +14,8 @@ Will be used if [tool.rope] is configured. | |||
|
|
|||
| [tool.rope] | |||
| split_imports = true | |||
| [tool.rope.autoimport] | |||
| underlined = false | |||
There was a problem hiding this comment.
I'd much prefer to keep a flat namespace here for the config keys. The vast majority of users are going to only have a small amount of rope config, creating multiple config sections just for a handful of configuration seems overkill and creates opportunities for errors. Most users wouldn't care/know about rope internals enough to intuitively know that autoimport is a separate package inside rope.
There was a problem hiding this comment.
IIRC, toml allows dotted keys. Maybe we can have our cake and eat it too, by recommending the dotted form in the examples section instead of the separate section:
[tool.rope]
autoimport.underlined = true
Motivation
Say you have a large environment with a package (IE: Pytorch) that you mark as a dependency. While you plan to use pytorch, its dependents consume a lot of unnessecary space. Additionally, it doesn't make sense to directly import those packages (since it breaks abstraction).
Description
Autodetects dependencies
This makes autoimport much much faster (1.33 sec vs 0.28 sec on a large env)
Pyproject.toml support
Will deprecate setting underlined per-function/directly w/o going through prefs (feedback?)
Checklist (delete if not relevant):