-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Filter duplicate imports when completing #2027
Filter duplicate imports when completing #2027
Conversation
The CI is fail in specific operating system version, But it seems that it was not introduced by my changes |
Thanks a lot! I'm happy to merge this once I have fixed the CI. I'm a bit confused how this started failing (because the last PR was fine). It's probably related to a Python 3.12 version upgrade. I will have to debug. |
After a small fix the tests are fine again (https://github.com/davidhalter/jedi/actions/runs/11364219966). Would it be possible for you to rebase? I'd prefer to merge once the CI is green. Thanks again, I think this is a useful addition and something that I found annoying as well! |
assert 'path' not in import_names(s) | ||
|
||
s = 'from os import path as pp, p' | ||
assert 'path' not in import_names(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a test here for
s = 'from os import chdir as path, p'
assert 'path' in import_names(s)
I feel like this is still wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will be wrong. I mentioned it above:
import alias will also be filtered Generally, there is unlikely to be a situation where an alias is named exactly the same as a specific original name.
If you think it unacceptable,I can parse this syntax and remove it from the filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I didn't read that.
If you think it unacceptable,I can parse this syntax and remove it from the filter
I think it is acceptable, but it is the kind of thing I always fix before merging. So if you don't do it, I have to :)
I think it's also a very easy fix (probably one simple if).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also know that this piece of software is used by millions of people, so issues like this one will appear sooner or later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done~
e5749f8
to
8f3292a
Compare
99d2e50
to
be6df62
Compare
Thanks a lot for the quick fixes and in general for the PR, good stuff! |
For the code snippet like:
When the cursor is after the letter F,the completion includes item "Field", But this import actually already existed before
Pycharm also will not complete:
So I create a new pull request to filter duplicate imports.
However, there are still some details that have not been implemented(I don't think it's very important),such as: