-
Notifications
You must be signed in to change notification settings - Fork 299
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
CodeBuilder - Provide 'casing friendly' default property RHS parameter name #5556
CodeBuilder - Provide 'casing friendly' default property RHS parameter name #5556
Conversation
Exposes a single method for generating a default Let\Set RHS property parameter IdentifierName.
Generates a property RHS parameter name based on the value of the property's IdentifierName.
Relocate and reformat the 'value' resource string from Resources.RubberduckUI to Resources.Refactorings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this is LGTM; there are just some nit-picks on naming and some commented out code in two tests.
|
||
public EncapsulateFieldCandidate(Declaration declaration, IValidateVBAIdentifiers identifierValidator) | ||
public EncapsulateFieldCandidate(Declaration declaration, IValidateVBAIdentifiers identifierValidator, Func<string, string> paramNameBuilder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of shortening parameter and variable names.
However, here it is good-enough as it is.
@@ -21,6 +21,8 @@ public class EncapsulateFieldTests : InteractiveRefactoringTestBase<IEncapsulate | |||
{ | |||
private EncapsulateFieldTestSupport Support { get; } = new EncapsulateFieldTestSupport(); | |||
|
|||
private static Func<string, string> Param = EncapsulateFieldTestSupport.ParamNameBuilder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer something like RhsParameterNameBuilder
, but it seems good-enough here in the tests.
RubberduckTests/Refactoring/EncapsulateField/EncapsulateFieldTests.cs
Outdated
Show resolved
Hide resolved
RubberduckTests/Refactoring/EncapsulateField/EncapsulateFieldTests.cs
Outdated
Show resolved
Hide resolved
RubberduckTests/Refactoring/EncapsulateField/EncapsulateUsingStateUDTTests.cs
Outdated
Show resolved
Hide resolved
RubberduckTests/Refactoring/EncapsulateField/EncapsulatedUDTFieldTests.cs
Outdated
Show resolved
Hide resolved
Renaming per suggestions. Also, clean-up of commented code and removal of non-required 'using' directives.
@MDoerner Thanks for reviewing this PR so it can be included in the next release. I preferred your naming suggestions - so they have been incorporated. All other comments addressed as well. |
Closes #5552.
This PR modifies the default property RHS parameter IdentifierName. The previous default ('value') creates casing issues(#5552). The PR proposes to resolve the issue by using a custom parameter IdentifierName for each generated property of the form "propertyIdentifierValue". (e.g.,
Property Let Foo(ByVal fooValue As Long)
).