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

Update documentation #788

Closed
wants to merge 4 commits into from
Closed

Update documentation #788

wants to merge 4 commits into from

Conversation

phreed
Copy link

@phreed phreed commented Mar 8, 2022

Documentation corrections an updates as mentioned in https://issues.apache.org/jira/b…rowse/LOG4J2-3429
Corrected typo in builder method 'withtFilter' -> 'withFilter'

…rowse/LOG4J2-3429

Also, corrected typo in builder method name 'withtFilter' -> 'withFilter'
@@ -214,7 +214,7 @@ public Filter getFilter() {
return filter;
}

public B withtFilter(Filter filter) {
public B withFilter(Filter filter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have mixed feelings about this change: yes, it would be nice to have a correctly spelled method, but the other one was already released and it is API.

Don't change the name of the method, deprecate it and add another one with the correct spelling.

Copy link
Author

Choose a reason for hiding this comment

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

I deprecated the misspelled method and created a new one with the correct spelling as suggested.

Copy link
Author

@phreed phreed Mar 9, 2022

Choose a reason for hiding this comment

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

There were two such cases.
What is the rule for naming methods?
When should the name be setFilter and when withFilter?
I just noticed that the appender was changed from withFilter to setFilter.
Should the same be done here?

Copy link
Member

Choose a reason for hiding this comment

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

I have mixed feelings about this change: yes, it would be nice to have a correctly spelled method, but the other one was already released and it is API.

Don't change the name of the method, deprecate it and add another one with the correct spelling.

Just add a new method with the correct name and deprecate the bogus one. Simple ;-)

@garydgregory
Copy link
Member

The "with" style is old, the "set" style is new. We already deprecated a bunch of "with" methods in favor of "set" methods.

@jvz jvz added the documentation Pull requests or issues that affect documentation label Jan 14, 2023
@vy vy deleted the branch apache:release-2.x February 28, 2023 15:02
@vy vy closed this Feb 28, 2023
@ppkarwasz
Copy link
Contributor

This was closed automatically by Github because we renamed the release-2.x branch to 2.x. Feel free to resubmit to the 2.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests or issues that affect documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants