Skip to content

Conversation

@pougetat
Copy link

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

@pougetat pougetat changed the title [WIP] Clear up log message when importing module [WIP] Clear up log message when importing module #8740 Jan 28, 2019
@pougetat pougetat changed the title [WIP] Clear up log message when importing module #8740 Clear up log message when importing module #8740 Jan 31, 2019
@pougetat pougetat force-pushed the better-export-message branch from f00a7e3 to 41c39a2 Compare February 3, 2019 20:27
@pougetat
Copy link
Author

pougetat commented Feb 4, 2019

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

@pougetat pougetat force-pushed the better-export-message branch from 41c39a2 to 268789a Compare February 6, 2019 22:43
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a 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.

Copy link
Collaborator

@rjmholt rjmholt left a 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
Copy link
Collaborator

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

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

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

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

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants