-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Clear up log message when importing module #8740 #8758
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
Conversation
f00a7e3 to
41c39a2
Compare
|
Hey @SteveL-MSFT , I updated my PR to reflect the discussion in the related issue. My CodeFactor error is in relation to code I haven't touched and I'm not too sure what to make of the PowerShell-CI-windows test failure since it doesn't seem related to my changes. If you have any hints ... :) |
src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs
Outdated
Show resolved
Hide resolved
41c39a2 to
268789a
Compare
SteveL-MSFT
left a comment
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.
You don't need to force push over your older commit. Add new commits with your changes as it makes it easier to review. When maintainers merge, they will squash anyways.
src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
Outdated
Show resolved
Hide resolved
rjmholt
left a comment
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.
This looks good to me. Made a single suggestion above.
Ideally we should test the case where the function is not imported if there's a way to read through the verbose stream.
| message = StringUtil.Format(Modules.ImportingFunction, prefixedName); | ||
| cmdlet.WriteVerbose(message); | ||
| } | ||
| else |
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.
Rather than putting this below an else, I think the best option is to invert the if condition above, like:
if (!SessionStateUtilities.MatchesAnyWildcardPattern(func.Name, functionPatterns, noPatternsSpecified))
{
string manifestFilename = Path.GetFileName(cmdlet.Context.PreviousModuleProcessed);
string moduleFilename = Path.GetFileName(sourceModule.Path);
message = StringUtil.Format(Modules.NotImportingFunction, func.Name, moduleFilename, manifestFilename);
cmdlet.WriteVerbose(message);
return;
}
// Golden path hereThere 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.
@rjmholt Yep I'm still on this. I've just been pretty busy lately :).
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.
Hmmm this doesn't actually work quite the way I hoped it would. Basically when loading the root module specified in a module manifest file there are two calls made to ImportFunctions().
- The first call is made as a consequence of calling LoadModule which will try to export AND import its members (thus the first call to ImportFunctions without any specified pattern so it says it can't export via the manifest file).
- The second call happens as a consequence of ImportModuleMembers().
The presence of these two calls means we get extra log messages which we don't want. I'm going to look into how to get around this.
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.
Yeah, the duplicate/subsequent calling of methods is a big problem in the module cmdlets (chiefly in ModuleCmdletBase). It gets quite complicated, and particularly when RootModule is specified, it discards a lot of information and calls itself recursively.
The potential for regressions is large, which is why nobody's refactored it yet. When I've found time, I've tried to write tests that will eventually cover enough of the functionality to rewrite this functionality properly. Certainly things have broken in this area before.
So essentially, don't feel too compelled to pursue this if it gets too complex. There are a number of outstanding work items in this part of the code that have been avoided for being too risky or too time consuming while it's in its present state.
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.
Thanks for the context. In that case i will probably close this pr in the coming days if I don't find a way around it. At least this tells me that there is work to be done in this part of the codebase (testing and refactoring).
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.
Definitely. It's one of the oldest parts of the codebase and very central to a lot of functionality (in subtle ways), so generally the strategy is to build out extensive testing for it and I'm hoping once we have regression testing in place we can refactor the logic into more contained, module pieces.
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.
Thanks for looking into this btw @pougetat -- we're always grateful for contributors like yourself.
PR Summary
Clearing up a log message when importing a module.
PR Context
The verbose output on a function says exported but it isn't actually exported (nor intended to be exported) from script module #8740.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.[feature]to your commit messages if the change is significant or affects feature tests