Grammar and typo fixes #14019

viliam-durina · 2024-11-25T08:41:40Z

A random collection while reading through the code.

shubhamvishu · 2024-11-28T18:34:29Z

lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java

@@ -184,7 +184,7 @@ List<DocumentsWriterPerThread> filterAndLock(Predicate<DocumentsWriterPerThread>
  /**
   * Removes the given DWPT from the pool unless it's already been removed before.
   *
-   * @return <code>true</code> iff the given DWPT has been removed. Otherwise <code>false</code>
+   * @return <code>true</code>, iff the given DWPT has been removed. Otherwise <code>false</code>


Maybe fix /iff/if/ since you are at it?

"Iff" is correct, it means "if and only if", see https://www.merriam-webster.com/dictionary/iff

Since this is all about syntax, grammar, etc, we might as well italicize iff since that is how it is generally done, as in the example you cited.

also, the comma you inserted is not correct (try replacing iff with "if and only if" - we wouldn't use a comma before "if").

msokolov

I started reviewing these one by one, but as I go I find that many of these changes insert unneeded commas and other agrammatical constructions that are sometimes worse than the original (which admittedly could stand to be improved). If you really want to work on this, I am happy to give out more grammar pedantry (it tickles some kind of weird obsession I have), but maybe it is not worth our time?

msokolov · 2024-12-01T17:12:34Z

lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java

@@ -184,7 +184,7 @@ List<DocumentsWriterPerThread> filterAndLock(Predicate<DocumentsWriterPerThread>
  /**
   * Removes the given DWPT from the pool unless it's already been removed before.
   *
-   * @return <code>true</code> iff the given DWPT has been removed. Otherwise <code>false</code>
+   * @return <code>true</code>, iff the given DWPT has been removed. Otherwise <code>false</code>


also, the comma you inserted is not correct (try replacing iff with "if and only if" - we wouldn't use a comma before "if").

msokolov · 2024-12-01T17:14:00Z

lucene/core/src/java/org/apache/lucene/index/IndexCommit.java

- * <p>Changes to the content of an index are made visible only after the writer who made that change
- * commits by writing a new segments file (<code>segments_N</code>). This point in time, when the
- * action of writing of a new segments file to the directory is completed, is an index commit.
+ * <p>Changes to the content of an index are made visible only after the writer which made that


sorry this is wrong. We could use that, but who is also correct - no need to "fix" it,

msokolov · 2024-12-01T17:15:57Z

lucene/core/src/java/org/apache/lucene/index/IndexCommit.java

- * commits by writing a new segments file (<code>segments_N</code>). This point in time, when the
- * action of writing of a new segments file to the directory is completed, is an index commit.
+ * <p>Changes to the content of an index are made visible only after the writer which made that
+ * change commits by writing a new segments file (<code>segments_N</code>). This point in time, when


OK this sentence is awkward, but what you've done here doesn't really address the core problem. I think it really would be most correct as: "The point in time (no comma) when ... is completed (no comma) is an index commit".

heh, I did read this as if the writer was a human; not sure why! I still think that is better than which not sure why exactly. I think this page describes the difference well: https://www.dictionary.com/e/that-vs-which/ -- if the restrictive clause is essential to the meaning of the sentence, as it is here, we use that.

msokolov · 2024-12-01T17:16:50Z

lucene/core/src/java/org/apache/lucene/index/IndexOptions.java

@@ -33,7 +33,7 @@ public enum IndexOptions {
  DOCS,
  /**
   * Only documents and term frequencies are indexed: positions are omitted. This enables normal
-   * scoring, except Phrase and other positional queries will throw an exception.
+   * scoring, except phrase and other positional queries will throw an exception.


I think I would prefer to avoid this ambiiguity by using PhraseQuery explicitly

msokolov · 2024-12-01T17:18:50Z

lucene/core/src/java/org/apache/lucene/index/IndexWriter.java

@@ -101,8 +101,8 @@
 * whether a new index is created, or whether an existing index is opened. Note that you can open an
 * index with {@link OpenMode#CREATE} even while readers are using the index. The old readers will
 * continue to search the "point in time" snapshot they had opened, and won't see the newly created
- * index until they re-open. If {@link OpenMode#CREATE_OR_APPEND} is used IndexWriter will create a
- * new index if there is not already an index at the provided path and otherwise open the existing
+ * index until they re-open. If {@link OpenMode#CREATE_OR_APPEND} is used, IndexWriter will create a


Again, not a great sentence, but fixing it requires a bigger rewrite; I don't think that this change is better (the "will" has no subject).

msokolov · 2024-12-01T17:19:45Z

lucene/core/src/java/org/apache/lucene/index/IndexWriter.java

@@ -123,13 +123,13 @@
 * IndexWriterConfig#setRAMBufferSizeMB}) or the number of added documents (see {@link
 * IndexWriterConfig#setMaxBufferedDocs(int)}). The default is to flush when RAM usage hits {@link
 * IndexWriterConfig#DEFAULT_RAM_BUFFER_SIZE_MB} MB. For best indexing speed you should flush by RAM
- * usage with a large RAM buffer. In contrast to the other flush options {@link
- * IndexWriterConfig#setRAMBufferSizeMB} and {@link IndexWriterConfig#setMaxBufferedDocs(int)},
+ * usage with a large RAM buffer. In contrast to the other flush options, in case of {@link


let's leave this as it was. It was OK.

viliam-durina · 2024-12-02T09:34:22Z

Thanks for the review 👍

I reverted all the disputed changes, agree we should not go into unproductive discussions. I added a few new ones, they are in a separate commit to simplify the review.

I'll only have a note to the writer ... who (#14019 (comment)) - "who" should be used only for persons, this writer is not a person. But I reverted it anyway.

msokolov

Thank you for all the fixes! I reviewed them all this time, and I think there are just a couple of changes I'd like to see before merging.

msokolov · 2024-12-02T14:08:11Z

lucene/core/src/java/org/apache/lucene/index/IndexWriter.java

@@ -141,7 +141,7 @@
 * soon as a new commit is done. Creating your own policy can allow you to explicitly keep previous
 * "point in time" commits alive in the index for some time, either because this is useful for your
 * application, or to give readers enough time to refresh to the new commit without having the old
- * commit deleted out from under them. The latter is necessary when multiple computers take turns
+ * commit deleted under their hands. The latter is necessary when multiple computers take turns


this sounds like an idiom of sort, but I've never heard it -- I think the existing phrasing is more idiomatic of the English I know anyway.

msokolov · 2024-12-02T14:11:26Z

lucene/core/src/java/org/apache/lucene/index/IndexWriterConfig.java

-   * some kind of sequence ID is available in the index.
+   * an arbitrary query to mark all documents that should survive the merge. This can be used, for
+   * example, to keep all document modifications for a certain time interval or the last N
+   * operations if some kind of sequence ID is available in the index.
   *
   * <p>Currently there is no API support to un-delete a soft-deleted document. In oder to un-delete


Can we fix "In oder" to be "In order"?

msokolov

Looks good, thank you!

msokolov · 2024-12-03T17:00:44Z

BTW I am fascinated by your github avatar pic! I feel like I have seen it before but not sure where? I tried image search but it does not find it. Is it something well known?

viliam-durina · 2024-12-03T17:31:09Z

BTW I am fascinated by your github avatar pic! I feel like I have seen it before but not sure where? I tried image search but it does not find it. Is it something well known?

It's an illustration from a children book by Albín Brunovský 😄

msokolov · 2024-12-03T18:08:23Z

wow those books would scare any child that I know!

viliam-durina · 2024-12-03T18:35:56Z

wow those books would scare any child that I know!

I don't know a child that would be scared by them 😄 .
Mine are not, and I wasn't when I was a child.

Grammar and typo fixes

7d793e5

A random collection while reading through the code.

viliam-durina force-pushed the grammar branch from 1a97703 to 7d793e5 Compare November 25, 2024 08:54

shubhamvishu reviewed Nov 28, 2024

View reviewed changes

msokolov reviewed Dec 1, 2024

View reviewed changes

viliam-durina added 3 commits December 2, 2024 09:57

More grammar fixes

c4b7fb4

Revert some changes

4e97cb8

Run gradlew tidy

4bf0676

Even more grammar fixes

b6132b4

msokolov approved these changes Dec 2, 2024

View reviewed changes

msokolov requested changes Dec 2, 2024

View reviewed changes

Address review comments

2685977

msokolov approved these changes Dec 3, 2024

View reviewed changes

msokolov merged commit 9c86bed into apache:main Dec 3, 2024
3 checks passed

viliam-durina deleted the grammar branch December 3, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grammar and typo fixes #14019

Grammar and typo fixes #14019

viliam-durina commented Nov 25, 2024

shubhamvishu Nov 28, 2024

viliam-durina Nov 28, 2024

msokolov Dec 1, 2024

msokolov Dec 1, 2024

msokolov left a comment

msokolov Dec 1, 2024

msokolov Dec 1, 2024

msokolov Dec 1, 2024

msokolov Dec 2, 2024

msokolov Dec 1, 2024

msokolov Dec 1, 2024

msokolov Dec 1, 2024

viliam-durina commented Dec 2, 2024 •

edited

Loading

msokolov left a comment

msokolov Dec 2, 2024

msokolov Dec 2, 2024

msokolov left a comment

msokolov commented Dec 3, 2024

viliam-durina commented Dec 3, 2024

msokolov commented Dec 3, 2024

viliam-durina commented Dec 3, 2024

Grammar and typo fixes #14019

Grammar and typo fixes #14019

Conversation

viliam-durina commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msokolov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viliam-durina commented Dec 2, 2024 • edited Loading

msokolov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msokolov left a comment

Choose a reason for hiding this comment

msokolov commented Dec 3, 2024

viliam-durina commented Dec 3, 2024

msokolov commented Dec 3, 2024

viliam-durina commented Dec 3, 2024

viliam-durina commented Dec 2, 2024 •

edited

Loading