-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Spelling #613
Spelling #613
Conversation
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
emm |
@jsoref, even though we appreciate your contribution for code clean-ups, it is quite time consuming for us to review every single line, in particular, given your changes span across hundreds of files and thousands of lines. Time is a pretty scarce resource among Log4j maintainers and we rather spend that on higher priority issues, e.g., bug triages & fixes, (killer?) features, etc. It would be a pity to get you discouraged from contributing, yet, we think such bulk code clean-ups are better done by committers without needing the attention of another developer. That said, we will be more than happy to see you participating in more functional Log4j stories. Would you consider either improving the documentation and/or picking up open Jira issues? This might also make your way to becoming a committer, where you can freely go wild with your code clean-ups. |
I'm not currently a log4j2 user. We happen to be using log4j, so, in theory, I have some potential interest in this project as opposed to just offering a general contribution to a project I've heard of – which I do regularly. I personally tend to check the health of a project by sending a change like this. (That you responded in under a day puts you into the healthy side by one of my measures.) I'm quite happy to respond to review comments, e.g. "only this directory", or "only comments", or "only code", or "drop these directories", or "squash". In a past life, I did a lot of bug triage. I don't have a particular interest to do that in my spare time. I do appreciate your offer and attempts to grow your community. Fwiw, I am an ASF contributor and an working on getting similar changes merged into other ASF projects. I'm also very patient, it doesn't matter to me if it takes a year to integrate changes – since I can do other work elsewhere in the interim. |
Josh, long time no see! It'd be awesome if you could either break up changes or leave PR comments on relevant areas to help with review. |
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 generally do leave an annotation of the interesting bits.
I've skipped commenting on local variable renames as they're generally uninteresting. Similarly, I've skipped most comment changes.
Happy to split.
@@ -1,7 +1,7 @@ | |||
/* | |||
* Licensed to the Apache Software Foundation (ASF) under one or more | |||
* contributor license agreements. See the NOTICE file distributed with | |||
* this work for additional inparserion regarding copyright ownership. | |||
* this work for additional information regarding copyright ownership. |
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.
This damaged content is original to the file and is in a license block, which I'd imagine is probably bad form per ASF.
@@ -29,7 +29,7 @@ | |||
@Tag("allocation") | |||
@Tag("functional") | |||
@Category(GarbageFree.class) | |||
public class GcFreeMixedSyncAyncLoggingTest { | |||
public class GcFreeMixedSyncAsyncLoggingTest { |
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.
Sigh, java classes require a rename -- I have a script to do that but generally forget to use it since most of the time I'm not touching java projects...
| log4j2.kubernetes.client.loggingInberval | spring.cloud.kubernetes.client.loggingInterval | 20s | Logging interval | | ||
| log4j2.kubernetes.client.loggingInterval | spring.cloud.kubernetes.client.loggingInterval | 20s | Logging interval | |
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.
This is user facing documentation which is actually wrong.
log4j.logger.org.apache.hadoop.crytpo.key.kms.server=ALL | ||
log4j.logger.org.apache.hadoop.crypto.key.kms.server=ALL |
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.
Google doesn't recognize this string outside here:
https://www.google.com/search?q=%22log4j.logger.org.apache.hadoop.crytpo.key.kms.server%22
"justification": "Removed deprectated method" | ||
"justification": "Removed deprecated method" |
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.
Probably developer facing documentation
* https://www.ralphgoers.com/post/log4j-1-compatibility-in-log4j-2[Log4j 1 Compatiblity in Log4j 2] | ||
* https://www.ralphgoers.com/post/log4j-1-compatibility-in-log4j-2[Log4j 1 Compatibility in Log4j 2] |
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.
This link description doesn't match the actual title of the web page.
The Log4j-1.2-api module of Log4j 2 provides compatiblity for applications using the Log4j 1 logging methods. As | ||
The Log4j-1.2-api module of Log4j 2 provides compatibility for applications using the Log4j 1 logging methods. As | ||
of Log4j 2.13.0 Log4j 2 also provides experimental support for Log4j 1.x configuration files. See | ||
link:manual/compatiblity.html[Log4j 2 Compatiblity with Log4j 1] for more information. | ||
link:manual/compatibility.html[Log4j 2 Compatibility with Log4j 1] for more information. |
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.
This developer facing link is bad, as the actual file is: src/site/markdown/manual/compatibility.md
|allowdLdapClasses|String|null|A comma separated list of fully qualified class names that may be accessed by LDAP. | ||
The classes must implement Serializable. Only applies when the JMS Appender By default only Java primative classes | ||
|allowedLdapClasses|String|null|A comma separated list of fully qualified class names that may be accessed by LDAP. | ||
The classes must implement Serializable. Only applies when the JMS Appender By default only Java primitive classes | ||
are allowed. | ||
|allowdLdapHosts|String|null|A comma separated list of host names or ip addresses that may be accessed by LDAP. | ||
|allowedLdapHosts|String|null|A comma separated list of host names or ip addresses that may be accessed by LDAP. | ||
By default only the local host names and ip addresses are allowed. | ||
|allowdJndiProtocols|String|null|A comma separated list of protocol names that JNDI will allow. By default only java, | ||
|allowedJndiProtocols|String|null|A comma separated list of protocol names that JNDI will allow. By default only java, |
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.
This developer facing documentation is wrong as these fields don't exist in these misspelled forms.
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.
Normally my tooling doesn't merge commits together, but this probably happened because of a merge conflict when I sorted my commits. Sorry about that.
public String doUpdate(String table, Map<String, String> params) { | ||
logger.entry(param); | ||
|
||
logger.debug(UPDATE_MARKER, new SQLMessage(SQLMessage.SQLType.UPDATE, table, parmas); | ||
logger.debug(UPDATE_MARKER, new SQLMessage(SQLMessage.SQLType.UPDATE, table, params); |
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.
This developer facing sample is broken
<item name="Configuration Archtecture" href="/manual/configuration.html#Architecture"/> | ||
<item name="Configuration Architecture" href="/manual/configuration.html#Architecture"/> |
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.
This probably means that the front facing ToC is bad. This makes it harder for users to find things (it's at least screen reader hostile, but also problematic for anyone using search).
I'll review this in more detail later. |
One typo that I noticed earlier was 'primative' appearing in a few places in one of the PRs related to the recent CVE issue. (https://github.com/apache/logging-log4j2/pull/608/files) -- since a lot of people will be reading these changes in particular, this typo would be a good one to start with - I can do a PR for just this one if people think that is a good idea |
@pjfanning That is one of my favorite words to misspell. :-) |
I don't care about credit. I make PRs to improve codebases. Feel free to cherry-pick whichever pieces/ideas you like (or reimplement). I've also made a corresponding set of changes for release-2.x: https://github.com/jsoref/logging-log4j2/commits/spelling-2.x I'm quite willing to make a subset of changes. The easiest thing would be to drop changes to |
This was closed automatically by Github because we renamed the |
I'm working on it, but your linters and I do not get along. |
Spotless can correct formatting violation: run Formatting checks bother everybody, but it beats conflicts caused by whitespace differences. |
|
There is a module without |
That's unhelpful. Please provide a full incantation instead of a random hint. |
@jsoref: in bash you run |
https://issues.apache.org/jira/browse/LOG4J2-3203
This PR corrects misspellings identified by the check-spelling action.
The misspellings have been reported at jsoref@6864e28#commitcomment-61673287
The action reports that the changes in this PR would make it happy: jsoref@db6552e
Note: this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.