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

Pass root path to FullyQualifiedNameProvider #867

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Conversation

amyreese
Copy link
Member

Summary

This allows FullyQualifiedNameProvider to work with absolute paths,
rather than assuming all paths given will be relative to the current
directory. This enables tools like Fixit to provide a root path, and
have the FullyQualifiedNameProvider correctly scope the final results
relative to that root path.

This does require that both the root path and the given file paths
match the other as relative or absolute, due to the
calculate_module_and_package helper comparing file paths relative
to the root path, but this seems like a reasonable tradeoff, and
unlikely to cause a problem in normal use cases.

Test Plan

Updated test cases. Works with in-progress Fixit2 changes.

@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 Feb 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2023

Codecov Report

Base: 94.79% // Head: 94.79% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (73e6daa) compared to base (8aebbb6).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #867   +/-   ##
=======================================
  Coverage   94.79%   94.79%           
=======================================
  Files         249      249           
  Lines       25831    25835    +4     
=======================================
+ Hits        24487    24491    +4     
  Misses       1344     1344           
Impacted Files Coverage Δ
libcst/_types.py 85.71% <100.00%> (+5.71%) ⬆️
libcst/helpers/module.py 100.00% <100.00%> (ø)
libcst/metadata/name_provider.py 98.64% <100.00%> (ø)
libcst/metadata/tests/test_name_provider.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@davidshepherd7
Copy link

FWIW I also ran into this bug and tried the same fix (passing root_path instead of "." in gen_cache, and this fix allowed me to use FullyQualifiedNameProvider as a METADATA_DEPENDENCIES in a codemod where before it was broken.

I was just about to start writing a PR for the same thing when I saw this one! 🎉 😍

Also: fixes #722 (someone else there had the same issue and the same solution).

@amyreese amyreese force-pushed the fqnp branch 3 times, most recently from 1b40cfd to 6ae4c37 Compare February 21, 2023 22:08
@amyreese amyreese requested review from zsol and lpetre February 21, 2023 22:25
@amyreese amyreese self-assigned this Feb 21, 2023
@amyreese amyreese changed the base branch from main to pr-867-base February 22, 2023 03:07
@amyreese amyreese changed the base branch from pr-867-base to main February 22, 2023 20:45
This allows FullyQualifiedNameProvider to work with absolute paths,
rather than assuming all paths given will be relative to the current
directory. This enables tools like Fixit to provide a root path, and
have the FullyQualifiedNameProvider correctly scope the final results
relative to that root path.

This does require that both the root path and the given file paths
match the other as relative or absolute, due to the
`calculate_module_and_package` helper comparing file paths relative
to the root path, but this seems like a reasonable tradeoff, and
unlikely to cause a problem in normal use cases.
@amyreese amyreese merged commit f9536b5 into Instagram:main Feb 22, 2023
@amyreese amyreese deleted the fqnp branch February 22, 2023 22:36
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