Skip to content

Add suggested correction to PSMissingModuleManifestField rule #515

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

Merged
merged 3 commits into from
May 3, 2016

Conversation

kapilmb
Copy link

@kapilmb kapilmb commented Apr 29, 2016

The rule is triggered whenever a module manifest doesn't contain module version. This commit sets the suggestedCorrections property to suggest a ModuleVersion field to the manifest.

Resolves #512


This change is Reviewable

Kapil Borle added 2 commits April 25, 2016 17:45
* Modifies the implmentation to add description and other minor changes
* Adds test cases to verify the correction extent of the rule
@raghushantha
Copy link
Member

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@raghushantha
Copy link
Member

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@lzybkr
Copy link
Member

lzybkr commented Apr 29, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Rules/MissingModuleManifestField.cs, line 78 [r1] (raw file):
I don't think you'll want this longer term - we can produce a HashtableAst in a configuration that doesn't start with @{ and you may want to have a similar diagnostic there too.


Rules/MissingModuleManifestField.cs, line 86 [r1] (raw file):
I think the correction should go at the end, not the start.


Comments from Reviewable

@daviwil
Copy link
Contributor

daviwil commented Apr 29, 2016

Since this rule seems to be specifically about module version, should you make the rule name specific to that as well or are you planning to look for other missing fields in the future?


Review status: all files reviewed at latest revision, 3 unresolved discussions.


Tests/Rules/AvoidUnloadableModuleOrMissingRequiredFieldInManifest.tests.ps1, line 29 [r1] (raw file):
Should you use $expectedCorrections in the 'Should' check here just for consistency?


Comments from Reviewable

@KirkMunro
Copy link
Contributor

My hopes (and what I believe was the intent) was that this rule would identify missing fields in future versions as well.


Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@kapilmb
Copy link
Author

kapilmb commented Apr 29, 2016

According to Test-ModuleManifest, a module manifest missing only the ModuleVersion field is an invalid manifest. And as of now this rule is just trying to warn the user of the invalidity of the module. However, going forward we would like to add other fields to this rule which can encode the "best practices" while authoring a module.


Review status: all files reviewed at latest revision, 3 unresolved discussions.


Rules/MissingModuleManifestField.cs, line 78 [r1] (raw file):
I will add that diagnostic - Can you please point me the resource where I can find the other ways I can produce a HashtableAst?


Rules/MissingModuleManifestField.cs, line 86 [r1] (raw file):
Sorry, but I am not able to see through as to why it should go at the end. Can you please explain why the correction should go at the end and not the start?


Comments from Reviewable

@kapilmb
Copy link
Author

kapilmb commented Apr 29, 2016

Rules/MissingModuleManifestField.cs, line 86 [r1] (raw file):
I can think of this - the editor doesn't need to displace all the text if we put the correction at the end. Is there any other reason why the correction should go at the end?


Comments from Reviewable

@lzybkr
Copy link
Member

lzybkr commented Apr 29, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions.


Rules/MissingModuleManifestField.cs, line 86 [r1] (raw file):
I had 2 thoughts - thinking generally about correcting missing entries in a hashtable - that appending is probably better than inserting - my thinking is that the user puts the most important stuff first, we're overriding that. The second thought was that I rarely see ModuleVersion as the first entry in a module manifest. It does happen, but it's not the norm.


Comments from Reviewable

* Insert ModuleVersion field at the end of the hashtable instead of the beginning
* Fix MissingModuleManifest test case
@kapilmb kapilmb merged commit f0bd654 into development May 3, 2016
@kapilmb
Copy link
Author

kapilmb commented May 3, 2016

@raghushantha @lzybkr @daviwil Thanks for the review.

@kapilmb kapilmb deleted the AddSuggestedCorrectionMissingMemberManifest branch May 3, 2016 23:19
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.

6 participants