X Tutup
Skip to content

Set correct PSProvider full name#11813

Merged
adityapatwardhan merged 7 commits intoPowerShell:masterfrom
iSazonov:fix-provider-drive-hidden2
Apr 27, 2020
Merged

Set correct PSProvider full name#11813
adityapatwardhan merged 7 commits intoPowerShell:masterfrom
iSazonov:fix-provider-drive-hidden2

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Feb 8, 2020 •
edited
Loading

PR Summary

Address #9840.

  • Set correct PSProvider full name at module load time.
  • Make PSProvider.FullName public

PR Context

In #8831 we added caching for ProviderInfo.FullName. This created a side effect.
PowerShell engine loads a module with name based on RootModule manifest property. Then it set explicitly correct module name based on the module (manifest) path. If the module contains a provider PowerShell engine loads and initializes the provider too. Full name of the provider is based on the module name and it is cached. So we should update the provider full name too that the fix explicitly does.

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 8, 2020
@iSazonov iSazonov added this to the GA-consider milestone Feb 8, 2020
@iSazonov iSazonov mentioned this pull request Feb 8, 2020
Closed
14 tasks
PoshChan
PoshChan reviewed Feb 8, 2020
View reviewed changes
PoshChan
PoshChan reviewed Feb 8, 2020
View reviewed changes
PoshChan
PoshChan reviewed Feb 8, 2020
View reviewed changes
@iSazonov iSazonov force-pushed the fix-provider-drive-hidden2 branch from e9511dd to 5230d80 Compare February 8, 2020 17:15
PoshChan
PoshChan reviewed Feb 8, 2020
View reviewed changes
PoshChan
PoshChan reviewed Feb 8, 2020
View reviewed changes
PoshChan
PoshChan reviewed Feb 8, 2020
View reviewed changes
rjmholt
rjmholt reviewed Feb 8, 2020
View reviewed changes
src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs Outdated
Copy link
Copy Markdown
Collaborator

@rjmholt rjmholt Feb 8, 2020

Choose a reason for hiding this comment

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

Given that you're using a foreach immediately below, would it be possible to remove the LINQ usage altogether?

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Feb 9, 2020

Choose a reason for hiding this comment

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

Yes, I removed the LINQ.

PoshChan
PoshChan reviewed Feb 9, 2020
View reviewed changes
rjmholt
rjmholt approved these changes Feb 9, 2020
View reviewed changes
@adityapatwardhan
Copy link
Copy Markdown
Member

adityapatwardhan commented Feb 10, 2020

@iSazonov Adding a new public API will need extra scrutiny for considering this for 7 GA.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Feb 11, 2020 •
edited
Loading

@adityapatwardhan I reverted FullName to internal and update test.

daxian-dbw
daxian-dbw requested changes Feb 11, 2020
View reviewed changes
@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Feb 11, 2020
@rjmholt rjmholt self-requested a review February 12, 2020 23:46
@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Mar 13, 2020

@daxian-dbw @rjmholt Could you please review?

@iSazonov iSazonov modified the milestones: GA-consider, 7.1.0-preview.1 Mar 13, 2020
@iSazonov iSazonov force-pushed the fix-provider-drive-hidden2 branch from 279cfcf to e3ed09b Compare March 29, 2020 06:46
@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Mar 29, 2020

@daxian-dbw Could you please continue?

@adityapatwardhan
Copy link
Copy Markdown
Member

adityapatwardhan commented Apr 8, 2020

@daxian-dbw Please re-review

daxian-dbw
daxian-dbw requested changes Apr 24, 2020
View reviewed changes
src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs Outdated
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think we probably should add the smart-ness into ProviderInfo.FullName to change the cached _fullName when the ModuleName property is changing.
Something like this:

private string _cachedModuleName = null;
internal string FullName
{
    get
    {
        string GetFullName(string name, string psSnapInName, string moduleName)
        { ... }

        if (_fullName != null && _cachedModuleName == ModuleName)
        {
            return fullName;
        }

        _cachedModuleName = ModuleName;
        return (_fullName = GetFullName(Name, PSSnapInName, ModuleName));
    }
}

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Apr 25, 2020

Choose a reason for hiding this comment

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

Done.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 24, 2020
@iSazonov iSazonov force-pushed the fix-provider-drive-hidden2 branch from e3ed09b to 47e0d30 Compare April 25, 2020 07:02
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 25, 2020
daxian-dbw
daxian-dbw approved these changes Apr 26, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan adityapatwardhan merged commit 86b6a9d into PowerShell:master Apr 27, 2020
@adityapatwardhan adityapatwardhan added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Apr 27, 2020
@iSazonov iSazonov deleted the fix-provider-drive-hidden2 branch April 28, 2020 03:23
@ghost
Copy link
Copy Markdown

ghost commented May 19, 2020

🎉v7.1.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

@rjmholt rjmholt mentioned this pull request Mar 2, 2021
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@rjmholt rjmholt Awaiting requested review from rjmholt

+1 more reviewer

@PoshChan PoshChan PoshChan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.3

Development

Successfully merging this pull request may close these issues.

6 participants

@iSazonov @adityapatwardhan @daxian-dbw @rjmholt @PoshChan @TravisEz13
X Tutup