Skip to content

Adds a rule to check if HelpMessage parameter attribute is non-null and non-empty. #443

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 7 commits into from
Feb 9, 2016

Conversation

kapilmb
Copy link

@kapilmb kapilmb commented Feb 9, 2016

Review on Reviewable

@raghushantha
Copy link
Member

Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions.


RuleDocumentation/AvoidNullOrEmptyHelpMessageAttribute.md, line 20 [r1] (raw file):
Use single quotes for constants


RuleDocumentation/AvoidNullOrEmptyHelpMessageAttribute.md, line 33 [r1] (raw file):
Single Quotes


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 24 [r1] (raw file):
Update comment


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 30 [r1] (raw file):
Update comment


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 43 [r1] (raw file):
have braces for all conditional statements. avoids future bugs here


Comments from the review on Reviewable.io

@quoctruong
Copy link

Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 60 [r1] (raw file):
Should we raise an error when namedArgument.ExpressionOmitted is true (assuming that [Parameter(HelpMessage)] does not work)?


Comments from the review on Reviewable.io

@kapilmb
Copy link
Author

kapilmb commented Feb 9, 2016

Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 60 [r1] (raw file):
As far as the PowerShell interpreter goes, it doesn't give any error for [Parameter(HelpMessage)].


Comments from the review on Reviewable.io

@kapilmb
Copy link
Author

kapilmb commented Feb 9, 2016

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 24 [r1] (raw file):
Done.


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 30 [r1] (raw file):
Done.


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 43 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@raghushantha
Copy link
Member

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


RuleDocumentation/AvoidNullOrEmptyHelpMessageAttribute.md, line 2 [r1] (raw file):
Severity is warning for this rule


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 85 [r1] (raw file):
Mark as Warning severity


Comments from the review on Reviewable.io

@kapilmb
Copy link
Author

kapilmb commented Feb 9, 2016

Review status: 5 of 9 files reviewed at latest revision, 4 unresolved discussions.


RuleDocumentation/AvoidNullOrEmptyHelpMessageAttribute.md, line 2 [r1] (raw file):
Done.


Rules/AvoidNullOrEmptyHelpMessageAttribute.cs, line 85 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@juneb
Copy link
Contributor

juneb commented Feb 9, 2016

With all due respect, why are we even writing this rule?
Here's a really old blog post about HelpMessage:
http://powershell.org/wp/2013/05/06/a-helpful-message-about-helpmessage/

@raghushantha
Copy link
Member

Reviewed 4 of 4 files at r2.
Review status: 8 of 9 files reviewed at latest revision, 4 unresolved discussions.


Comments from the review on Reviewable.io

@raghushantha
Copy link
Member

:lgtm:


Review status: 8 of 9 files reviewed at latest revision, 4 unresolved discussions.


Comments from the review on Reviewable.io

@raghushantha
Copy link
Member

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from the review on Reviewable.io

@raghushantha
Copy link
Member

Hi June.
Thank you for the pointer about using HelpMessage.

This rule does not encourage to use the attribute, but ensures that when it is used, the value is set to something useful.

Absence of HelpMessage attribute does not trigger the rule.

It is possible that in the future PowerShell runtime can use the value in a more meaningful way.

Thanks,
Raghu

@juneb
Copy link
Contributor

juneb commented Feb 9, 2016

Thanks. What harm does an empty HelpMessage attribute cause? Remember that we don't check for any help for parameters.

@raghushantha
Copy link
Member

Empty value results in a obscure interpreter error.

function Test-EmptyHelpMessage
{
[CmdletBinding()]
param (
[parameter(HelpMessage = "")]
[string]
$String
)
}

Test-EmptyHelpMessage
Cannot process argument because the value of argument "value" is not valid. Change the value of the "value" argument and
run the operation again.
At line:5 char:9

  •     [parameter(HelpMessage = "")]
    
  •     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    
    • CategoryInfo : InvalidOperation: (:) [], RuntimeException
    • FullyQualifiedErrorId : PropertyAssignmentException

@juneb
Copy link
Contributor

juneb commented Feb 9, 2016

Got it. Thanks.

kapilmb pushed a commit that referenced this pull request Feb 9, 2016
…Empty

Adds a rule to check if HelpMessage parameter attribute is non-null and non-empty.
@kapilmb kapilmb merged commit eb6a259 into development Feb 9, 2016
@kapilmb kapilmb deleted the AddRuleHelpMessageShouldNotBeEmpty branch February 9, 2016 21:57
@KirkMunro
Copy link
Contributor

Also @juneb, regarding your blog post, I know people who have used the !? feature with mandatory parameters before.

HelpMessage can be very useful metadata for tools built on top of PowerShell because it can provide a brief description of what is expected in a mandatory parameter, which could be displayed as part of Intellisense for that parameter. Parameter help will likely be much more descriptive than HelpMessage. I'm pretty sure some PowerShell tools have used this information in the past, but it's been long enough that I can't say for sure. For the tools I managed, if they weren't doing it already, it was something that was on the list of enhancements to add.

@kapilmb kapilmb restored the AddRuleHelpMessageShouldNotBeEmpty branch February 16, 2016 22:30
@kapilmb kapilmb deleted the AddRuleHelpMessageShouldNotBeEmpty branch February 16, 2016 22:30
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