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

Supports passing no providers to FullRepoMetadataConfig. #182

Closed
wants to merge 3 commits into from

Conversation

lisroach
Copy link
Contributor

@lisroach lisroach commented Apr 5, 2021

Summary

#181

Allowing passing no providers to FullRepoMetadata. There is already a fallback if the provider is broken where a placeholder cache is used, so I used the same codepath to treat empty providers and exceptions the same way.

There is currently only one placeholder cache type so I hardcoded TypeInferenceProvider, but if we want this to be more flexible I can try to build support for multiple.

Test Plan

Added a unit test. Tested manually and exception no longer fires and linters work correctly.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 5, 2021
@thatch
Copy link
Contributor

thatch commented Apr 5, 2021

Seems reasonable; check pyre and is there any other kind of full repo metadata other than TypeInferenceProvider that we might need to support?

@lisroach
Copy link
Contributor Author

lisroach commented Apr 5, 2021

I don't know why pyre is failing, I cannot get it to repro locally :(

I did some digging to try to figure out what the plan was with the PLACEHOLDER_CACHES and looks like it exists mostly because there is a bug in TypeInferenceProviders that triggers if cache is empty: #31 (comment)

I'm going to take a look at fixing that and reconfiguring this to not use placeholders at all if possible.

@lisroach
Copy link
Contributor Author

lisroach commented Apr 5, 2021

Created Instagram/LibCST#475 for the LibCST bug

@jimmylai
Copy link
Contributor

jimmylai commented Apr 6, 2021

Pyre failed due to a type error was found in fixit/rules/cls_in_classmethod.py:286:26 and that's related to the new LibCST release.
https://github.com/Instagram/Fixit/runs/2271833508
The same issue happens on master branch and it's better to fix it before merging new PRs.
FlattenSentinel is added in LibCST 0.3.18 and we need to update the type annotation of report().

replacement: Optional[Union[cst.CSTNode, cst.RemovalSentinel]] = None,

@lisroach
Copy link
Contributor Author

lisroach commented Apr 6, 2021

Thanks @jimmylai! No rush, I want to wait on this PR until after Instagram/LibCST#476 is merged and released. I think it will clean up the code here nicely.

@lisroach
Copy link
Contributor Author

@zsol do you know when LibCST will be released again? I'd like to get this fix in :) Happy to help if I can!

@lisroach
Copy link
Contributor Author

There has been a new LibCST release so I am back looking at this. The bug in TypeInferenceProviders has been fixed but there is still a problem here: https://github.com/Instagram/LibCST/blob/e759ca8290585cf3badcd22ac5e1fdbc5e4f9e13/libcst/metadata/base_provider.py#L69

When there is a TimeoutError from using Pyre we trigger this exception that says Cache is required for initializing TypeInferenceProvider.

@amyreese
Copy link
Member

amyreese commented Oct 7, 2022

Hey there! We appreciate your contributions, but we're in the process of making some large changes to the core of Fixit. We will try to have more info about the direction we're heading soon, but in the mean time, we are closing all outstanding PR's from before we started this work. Thank you for your understanding.

@amyreese amyreese closed this Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants