Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

Undo loop optimization #2077

Merged
merged 9 commits into from
Jun 19, 2020
Merged

Undo loop optimization #2077

merged 9 commits into from
Jun 19, 2020

Conversation

MikhailArkhipov
Copy link

Undo #1686. Basically fork right before the PR then cherry pick changes on top. Also removes caching services.

Fixes #1988 and probably some others - to be verified.
Some tests became less stable due to old loop resolution issues.

@PhilipMay
Copy link

This is great. Many thanks. I hope #1988 can be fixed soon.

@jakebailey
Copy link
Member

There seem to be some regressions in the __all__ handling. Might be something that was over-reverted, since I know the loop dependency stuff was using it to some extent.

@MikhailArkhipov
Copy link
Author

Minimized differences and ported fixes from PRs that were not taken due to compatibility problems with old code.

@MikhailArkhipov
Copy link
Author

What happened to __all__? Yes, loop resolution can definitely affect that. Theoretically it is possible to build off commit right before the loop one and compare if behavior is the same.

@jakebailey
Copy link
Member

jakebailey commented Jun 17, 2020

If you run the whole test suite, many of the __all__ related tests fail.

@MikhailArkhipov
Copy link
Author

Ran full stack, mine's are clean except one import ordering (it uses os and path which have loops, I guess I need to update the test).

image

image

@jakebailey
Copy link
Member

Those LS results appear to be cached; if I run them I get:

image

@MikhailArkhipov
Copy link
Author

MikhailArkhipov commented Jun 17, 2020

Those are not cached, they turn this way when you re-run individual tests. HoverSpanCheck was a known problem, typically passes on re-run. In MissingImports SymbolOrdering2 failed, very long test. Fixed.

What fails for you in highlight and imports? Are you on 3.8 or higher?

@jakebailey
Copy link
Member

Well, this is embarrassing. I'm still on your undo branch. I didn't see that this PR was being made from a branch on the repo itself rather than a fork. I'll retest.

@jakebailey
Copy link
Member

Yeah, now that I run the actual code it passes... 😄

I'll spot check this on some real code.

@MikhailArkhipov
Copy link
Author

This should be off my fork, u2 branch

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Checked it with a few projects, things appear to work as they do now (just without the loop messages, of course).

@MikhailArkhipov MikhailArkhipov merged commit 838ba78 into master Jun 19, 2020
@PhilipMay
Copy link

PhilipMay commented Jun 19, 2020

Is these anything that can be done on Code comment level so this bug will not be introduced again? Are there regression tests possible?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find all References does not search in subdirectories
3 participants