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

Support auto-import on snake_cased varaiables. #703

Closed
giyyapan opened this issue Dec 8, 2020 · 5 comments
Closed

Support auto-import on snake_cased varaiables. #703

giyyapan opened this issue Dec 8, 2020 · 5 comments
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@giyyapan
Copy link

giyyapan commented Dec 8, 2020

We have some flask apps that creates instances in a file and import it from other files. Please see the example:

In base.py

from flask import Flask
from flask_sqlalchemy import SQLAlchemy as FlaskSQLAlchemy

app = Flask()
db = FlaskSQLALchemy(app)

In api.py

from base import db

db.session.query(XXX)

Pylance's auto-import now suggests Classes, functions, UPPER_SNAKE_CASED variables, but no lower_snake_cased variables (like db). This would be a big problem for our code style (and this is a commonly used code style for flask users), and we'd have to manually write tons for imports by hand!

We tried to put those variables we'd like to import from other files into the __all__ list, but Pylance seems to ignoring it.

I assume this is an as-designed behaviour because I can image others might say this is the correct behaviour, but it makes us very hard to adapt using our current code base.

So I wish Pylance can have one of these improvements:

  1. Providing a config option to enable/disable auto-imports on snake_cased variable (may not include private variables starting with "_").
  2. Simply respect the __all__ variable (or some other magic variable that indicates which variable we would like to export).
  3. Privide an "Manual Search&Import" command, who uses an more aggressive stratege to search possible imports accross the project (like PyCharm).

Thanks!

@github-actions github-actions bot added the triage label Dec 8, 2020
@giyyapan
Copy link
Author

giyyapan commented Dec 8, 2020

Recently I'm trying very hard to make all of my colleague move from PyCharm to VSCode, but the auto-import related behaviours in VSCode are pushing them back to PyCharm.

According to my own experience, PyCharm is actually not doing a very good job about auto-import, but it have a well-implemented "manual import" actions, which uses a much aggresive stratege and searches all the possible match through the whole project and you can always find what you want even these action may take longer to complete. Even it might takes 3~5s to resolve import candidates in our project, it's still much faster than manually write those imports by hand.

I think auto-import is a feature that very hard to tweak, so maybe adding an "manual import" action that use a slow but "searches everything" stratege like PyCharm is a good way to go, because it can make sure everyone get what they want to import eventually.

@judej
Copy link
Contributor

judej commented Dec 8, 2020

@giyyapan, thanks for the report.

Could you please try using the Insiders Build (#623) which has the indexer enabled that should solve this problem.

Please let me know if this fixes the problem

thanks

@judej judej added the waiting for user response Requires more information from user label Dec 8, 2020
@github-actions github-actions bot removed the triage label Dec 8, 2020
@giyyapan
Copy link
Author

@judej, thanks for your reply.

I've tried the insiders version but snake_cased variables are still can't be auto imported.

Not sure if I did anything wrong, my Pylance version looks like this:
image

And I also tried the python.analysis.indexing setting, no luck.

But the workspace symbol search is functioning though, not sure If that proves the indexer is working.
image

@jakebailey jakebailey added needs investigation Could be an issue - needs investigation and removed waiting for user response Requires more information from user labels Dec 10, 2020
@heejaechang
Copy link
Contributor

heejaechang commented Mar 11, 2021

@giyyapan we explicitly filtering out lower case variables from the auto-import, that is why you are not seeing it. we are doing it since it bloats the completion list too much if we include all lower variables defined blindly.

I like this idea

Simply respect the all variable (or some other magic variable that indicates which variable we would like to export).

but not sure whether we want to show it in auto-import or "add import (code action/light bulb)" only.

unlike code action, auto-import can interfere with typing, so if we can, we don't want to add more entries in the completion.

@heejaechang heejaechang added fixed in next version (main) A fix has been implemented and will appear in an upcoming version and removed in backlog needs investigation Could be an issue - needs investigation labels Mar 31, 2021
@heejaechang
Copy link
Contributor

this is now fixed and lower case variable in db will show up in code action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests

5 participants