-
Notifications
You must be signed in to change notification settings - Fork 382
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
Fix compatibility profile query API so that aliases referring to other modules appear #1194
Conversation
Not sure why the git diff is so large. I tried making the changes in that file again but the git diff is still huge. No encoding change either. |
@rjmholt Using a whitespace ignoring diff, it is only a magnitude of ~ 100 lines changed, maybe there were some tabs in the original code (please don't tell me you're using tabs 😜) |
Shouldn't be -- I press Tab but that should expand to 4 spaces. Either way I wrote the file originally. Anyway, I tried redoing the changes in a fresh commit and that didn't work... But the psm1 changes just delete the alias table stuff. |
PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Query/Modules/ModuleData.cs
Outdated
Show resolved
Hide resolved
PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Query/Modules/ModuleData.cs
Outdated
Show resolved
Hide resolved
PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Query/Modules/ModuleData.cs
Outdated
Show resolved
Hide resolved
PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Query/RuntimeData.cs
Outdated
Show resolved
Hide resolved
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.
Looks, Ok, just the questions around case insensitive dictionaries and one optional style comment about using named tuples
@JamesWTruher I've just added some extra tests to capture aliases that are created outside the module of the command they refer to. The only ones I'm aware of are in the engine, and I dealt with them a different way before this change, so the tests will pass in both cases. But they do test that aliases defined elsewhere are still picked up. |
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.
looks fine to me (once the merge conflicts are addressed), thanks for adding more tests!
Ok I've rebased now. Let's see what the tests say |
PR Summary
Previously because of bad assumptions I made, an alias in a module that aliases a command outside that module would just be omitted from that module's query API.
I originally "fixed" this by building a table of aliases at profiling time and putting the aliases back into the modules of the commands they referenced. Not only was that wrong, but it also made profiling much slower.
Functionally, rules will see no differences and there are no breaking changes in the API, BUT this fixes the alias thing and should speed up profiling significantly (the latter being something I need).
PR Checklist
.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.