Skip to content

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.

Sign up for GitHub

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

Closed
pougetat wants to merge 2 commits into from
Closed

Clear up log message when importing module #8740 #8758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
268789a
Clear up log message when importing module
throwaway774 Jan 27, 2019
cecca54
Fixing PR comment
throwaway774 Feb 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7356,6 +7356,13 @@ private static void ImportFunctions(FunctionInfo func, SessionStateInternal targ
message = StringUtil.Format(Modules.ImportingFunction, prefixedName);
cmdlet.WriteVerbose(message);
}
else
Copy link
Collaborator

@rjmholt rjmholt Feb 7, 2019

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 here

Copy link
Author

@pougetat pougetat Feb 15, 2019

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 :).

Copy link
Author

@pougetat pougetat Feb 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjmholt

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.

Copy link
Collaborator

@rjmholt rjmholt Feb 19, 2019

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.

Copy link
Author

@pougetat pougetat Feb 19, 2019 •
edited
Loading

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).

Copy link
Collaborator

@rjmholt rjmholt Feb 19, 2019

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.

Copy link
Collaborator

@rjmholt rjmholt Feb 19, 2019

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.

{
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);
}
}

private static void SetCommandVisibility(bool isImportModulePrivate, CommandInfo command)
Expand Down
3 changes: 2 additions & 1 deletion src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1566,7 +1566,8 @@ internal static void ExportModuleMembers(
if (SessionStateUtilities.MatchesAnyWildcardPattern(entry.Key, functionPatterns, false))
{
sessionState.ExportedFunctions.Add(entry.Value);
string message = StringUtil.Format(Modules.ExportingFunction, entry.Key);
string filename = Path.GetFileName(sessionState.Module.Path);
string message = StringUtil.Format(Modules.ExportingFunction, entry.Key, filename);
cmdlet.WriteVerbose(message);
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/System.Management.Automation/resources/Modules.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@
<data name="ImportingFunction" xml:space="preserve">
<value>Importing function '{0}'.</value>
</data>
<data name="NotImportingFunction" xml:space="preserve">
<value>Not exporting function '{0}' from '{1}' via '{2}'.</value>
</data>
<data name="ImportingCmdlet" xml:space="preserve">
<value>Importing cmdlet '{0}'.</value>
</data>
Expand All @@ -307,7 +310,7 @@
<value>Exporting cmdlet '{0}'.</value>
</data>
<data name="ExportingFunction" xml:space="preserve">
<value>Exporting function '{0}'.</value>
<value>Exporting function '{0}' from '{1}'.</value>
</data>
<data name="ExportingAlias" xml:space="preserve">
<value>Exporting alias '{0}'.</value>
Expand Down