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
Fix misspelled words in comments, error messages, and test code #32792
Conversation
Pinging @elastic/es-docs |
After a few days, this pull request has become having a conflict with other merged changes. Thus, I've rebased on the |
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 looks good. I spotted a handful of changes that are still needed. I'm also not sure whether we should split this up into smaller sets of changes as I worry about merge conflicts and ease of backporting.
@@ -1063,7 +1063,7 @@ public int size() { | |||
} | |||
|
|||
/** | |||
* returns the history uuid the store points at, or null if not existant. | |||
* returns the history uuid the store points at, or null if not existent. |
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 think not existent
should be nonexistent
.
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.
Thanks. I've fixed it.
@@ -85,12 +85,12 @@ public int estimatedNumberOfOperations() { | |||
return numberOfOperations; | |||
} | |||
|
|||
/** the size of the generations in the translog that weren't yet to comitted to lucene */ | |||
/** the size of the generations in the translog that weren't yet to committe to lucene */ |
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.
committed
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.
Thanks. I've fixed this and the same one in the same file as well.
public long getUncommittedSizeInBytes() { | ||
return uncommittedSizeInBytes; | ||
} | ||
|
||
/** the number of operations in generations of the translog that weren't yet to comitted to lucene */ | ||
/** the number of operations in generations of the translog that weren't yet to committe to lucene */ |
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.
committed
@@ -51,7 +51,7 @@ public RestGetRepositoriesAction(Settings settings, RestController controller, S | |||
|
|||
@Override | |||
public String getName() { | |||
return "get_respositories_action"; | |||
return "get_repositories_action"; |
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.
Although this is a valid correction, this is also a breaking change. I think it only affects the REST Usage API, but I think this should be reverted and fixed separately.
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.
Reverted and added a FIXME comment there. If the comment is unnecessary, I will remove it.
@@ -132,7 +132,7 @@ public String getName() { | |||
hasToString(containsString( | |||
"request [/] contains unrecognized parameters: " + | |||
"[flied] -> did you mean [field]?, " + | |||
"[respones_param] -> did you mean [response_param]?, " + | |||
"[response_param] -> did you mean [response_param]?, " + |
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.
Lol. This mis-spelling is intentional.
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.
Oops, I reverted it.
Please don't rebase this again, because rebasing loses inline comments from reviews. The preferred way to keep this in line with |
@DaveCTurner Thank you for your review. Since I have time to work on this now, I will fix the parts you pointed out very soon. Also, I don't rebase this branch anymore. |
@DaveCTurner I've pushed a commit which reflects the updates. Could you review them again when you have time? 🙇 |
@@ -51,7 +51,8 @@ public RestGetRepositoriesAction(Settings settings, RestController controller, S | |||
|
|||
@Override | |||
public String getName() { | |||
return "get_repositories_action"; | |||
// FIXIME: the typo, just modifying the response will bring a breaking change to the REST Usage API, see also PR #32792 |
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.
Better not to add this. Also FIXME
contains a typo ;)
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.
Oh... >typo
Thanks, I will remove the line.
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.
Removed.
@elasticmachine test this please |
@elasticmachine test this please |
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 did another pass and found a few more things to fix. Additionally I pinged others for second opinions about three changes.
@@ -32,7 +32,7 @@ PUT _snapshot/my_hdfs_repository | |||
"type": "hdfs", | |||
"settings": { | |||
"uri": "hdfs://namenode:8020/", | |||
"path": "elasticsearch/respositories/my_hdfs_repository", | |||
"path": "elasticsearch/repositories/my_hdfs_repository", |
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.
@jbaiera I can't judge the impact of this change. Ok with you?
@@ -372,7 +372,7 @@ GET /test_index/_search | |||
"percolate" : { | |||
"field" : "query", | |||
"document" : { | |||
"body" : "Bycicles are missing" | |||
"body" : "Bicycles are missing" |
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.
@martijnvg I think this change is ok but just checking with you that it wasn't deliberately misspelled for the example.
@@ -122,7 +122,7 @@ private void testCancel(String action, AbstractBulkByScrollRequestBuilder<?, ?> | |||
logger.debug("waiting for updates to be blocked"); | |||
boolean blocked = awaitBusy( | |||
() -> ALLOWED_OPERATIONS.hasQueuedThreads() && ALLOWED_OPERATIONS.availablePermits() == 0, | |||
1, TimeUnit.MINUTES); // 10 seconds is usually fine but on heavilly loaded machines this can wake a while |
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.
s/wake/take/
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.
Also fixed.
@@ -39,7 +39,7 @@ public RestGetCategoriesAction(Settings settings, RestController controller) { | |||
|
|||
@Override | |||
public String getName() { | |||
return "xpack_ml_get_catagories_action"; | |||
return "xpack_ml_get_categories_action"; |
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 also a breaking change, so needs to be done separately.
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.
Reverted.
@@ -40,7 +40,7 @@ public void testResolveSingleValuedAttributeFromCachedAttributes() throws Except | |||
new Attribute("cn", "Clint Barton"), | |||
new Attribute("uid", "hawkeye"), | |||
new Attribute("email", "clint.barton@shield.gov"), | |||
new Attribute("memberOf", "cn=staff,ou=groups,dc=exmaple,dc=com", "cn=admin,ou=groups,dc=exmaple,dc=com") | |||
new Attribute("memberOf", "cn=staff,ou=groups,dc=example,dc=com", "cn=admin,ou=groups,dc=example,dc=com") |
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.
@tvernum this fix looks ok to me, but just checking with you that it wasn't a deliberate typo?
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.
No, this wasn't deliberate.
@@ -37,7 +37,7 @@ setup: | |||
} | |||
|
|||
--- | |||
"Test start non-existant job": | |||
"Test start non-existent job": |
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.
nonexistent
needs no hyphen
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.
Fixed.
@@ -37,7 +37,7 @@ setup: | |||
} | |||
|
|||
--- | |||
"Test stop non-existant job": | |||
"Test stop non-existent job": |
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.
nonexistent
needs no hyphen
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.
Fixed.
@@ -119,7 +119,7 @@ public String getName() { | |||
final HashMap<String, String> params = new HashMap<>(); | |||
params.put("consumed", randomAlphaOfLength(8)); | |||
params.put("flied", randomAlphaOfLength(8)); | |||
params.put("respones_param", randomAlphaOfLength(8)); | |||
params.put("response_param", randomAlphaOfLength(8)); |
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 misspelling was deliberate - it's testing the "did you mean" functionality.
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.
Reverted.
@elasticmachine test this please |
@DaveCTurner I just resolved the conflict with the latest revision. If there is something I can do for this pull request, please let me know at any time. |
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 LGTM, thanks so much for your efforts. I will start a full CI run now.
@elasticmachine test this please |
@elasticmachine test this please |
@elasticmachine test this please. |
it seems like this got outdated and there are some merge conflicts. What do you suggest to do @DaveCTurner ? Do you plan on getting back to it? |
Thank you for your response. I am fine even if you close this PR without merging due to lack of time for it. I understand most of the changes included in this PR are not crucial for the project. |
Apologies, I forgot to assign this to myself before going on holiday so it dropped off my list. I will try and bring it in line with master again. |
@elasticmachine test this please |
@elasticmachine test this please |
thanks so much @DaveCTurner !!! |
@DaveCTurner Thank you for merging this 👍 |
Fixing typos is sometimes very hard. It's not so easy to visually review them. Recently, I discovered a very useful tool for it, misspell.
This pull request fixes minor typos detected by misspell except for the false positives. If you would like me to work on other files as well, let me know.
gradle check
?