Skip to content
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

add support for withHelper for config field #264

Merged
merged 1 commit into from
Jan 22, 2021
Merged

Conversation

skotambkar
Copy link
Contributor

Updates Config field to support withHelper option it partially supports. The withHelper option is used to generate a With-Helper function for a Config Field.

}

@Override
public int hashCode() {
return Objects.hash(getName(), getType(), getDocumentation());
return Objects.hash(getName(), getType(), getDocumentation(), withHelper());
Copy link
Member

Choose a reason for hiding this comment

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

Should this follow the getter naming pattern? getWithHelper?

Copy link
Contributor Author

@skotambkar skotambkar Jan 20, 2021

Choose a reason for hiding this comment

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

I think with is self sufficient here, adding get seems redundant.

Copy link
Member

@skmcgrail skmcgrail Jan 22, 2021

Choose a reason for hiding this comment

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

The pattern for retrieving private fields in java is to prefix with get<FieldName>. It's ambiguous name makes it sound like a setter instead, or a builder helper.

Copy link
Contributor

@jasdel jasdel Jan 22, 2021

Choose a reason for hiding this comment

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

given the java naming scheme probably better to add a get prefix since its the java style. When this was original added, I wasn't following the Java naming, and used the implicit behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, updated!

@skotambkar skotambkar merged commit 0df1483 into main Jan 22, 2021
@jasdel jasdel deleted the WithHelperAddition branch January 22, 2021 20:23
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.

3 participants