14025 fix script name checking #14030
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanSheps
reviewed
Oct 12, 2023
|
Can we just use a regex to capture the portion of the file path preceding the dot? Seems like that would be sufficient. |
jeremystretch
requested changes
Oct 17, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #14025
@jeremystretch the module name checking is a bit verbose, not sure if there is a simpler way to do this.
The issue is if you have the script "test.py" module comes in as "test" and file_path is "test.py" which is why the startswith is there. If you remove startswith it won't match. I think it would be difficult at this point to change module to keep the full name with .py as I think it is stored that way.
Ideally if we could remove a potential ".py" from file_path in the filter clause but I don't think there is a way to do that. it would also work to change how the scripts are stored to include the full name but that would require a data migration with potentially incomplete historical information.