Change method name 'desc' to 'setDescription', 'size' to 'getNumViolations'#1957
Change method name 'desc' to 'setDescription', 'size' to 'getNumViolations'#1957pdhung3012 wants to merge 2 commits into
Conversation
Generated by 🚫 Danger |
jsotuyod
left a comment
There was a problem hiding this comment.
@pdhung3012 thanks for the PR and taking your time to work on PMD.
However, this is not a change we can accept.
Firstly, the PR includes 2 completely separate changes, so it should be split into 2 PRs. However, both of these changes are changing the names of public methods, meaning this is a breaking API change and something we can't do without:
- Properly deprecating and documenting this on a minor release to give users proper time to accommodate.
- Actually getting rid of deprecated methods on the next major release (7.0.0)
And finally, I'm not particularly in favor of changing desc for setDescription. As you stated, this is a builder, which provides a fluent API. A setter should, by convention return void. Moreover, changing this method alone would be completely inconsistent with all other methods in property builders, such as defaultValue.
I think the change on Report however is positive. Please consider submitting a PR with only that change, but considering:
- The old
size()method should not be removed, but simply@Deprecated, and have it internally call the new non-deprecated method. - Document this change on https://pmd.github.io/pmd-6.17.0/pmd_next_major_development.html#list-of-currently-deprecated-apis (this file)
|
Thanks, I will close this PR and create a new PR. |
Thank you. I created a new PR at #1960 |
The class PropertyBuilder is used to represent PropertyBuilder. This method named 'desc' is to specify the description of this property. Thus, the method name 'setDescription' is more intuitive than 'desc'.
The class PropertyBuilder is used to represent Report. This method named 'size' is to get number of violations. Thus, the method name 'getNumViolations' is more intuitive than 'size'.