Skip to content

Conversation

risdenk
Copy link
Contributor

@risdenk risdenk commented Feb 25, 2022

https://issues.apache.org/jira/browse/SOLR-14920

This is for solr core test files only. See PR #705 for non-test files.

Copy link
Contributor Author

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

First 100 findings :D reviewed about ~25% of the files but less than 25% of the lines changed (I skipped files with >1000 lines changed to start).

Copy link
Contributor Author

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

Almost halfway through

Copy link
Contributor Author

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

Almost 100 more comments :) getting there

Copy link
Contributor Author

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

Another chunk of files reviewed

Copy link
Contributor Author

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

Final part of first pass review

@risdenk risdenk force-pushed the SOLR-14920-core-test branch from a4351d9 to 89b7f83 Compare February 28, 2022 20:43
@risdenk risdenk marked this pull request as ready for review February 28, 2022 21:34
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

+1 thanks Kevin

@risdenk risdenk merged commit 73e64a9 into apache:main Mar 1, 2022
@risdenk risdenk deleted the SOLR-14920-core-test branch March 1, 2022 20:37
Copy link

@ErickErickson ErickErickson left a comment

Choose a reason for hiding this comment

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

As before, nothing I found is functionally significant.

I was moving right along doing all the files starting with a particular letter, knocking of 6 or 8 at a time... until I hit the "T"s ;)

There are a number of places where a string with a comment-to-EOL and the
comma for an additional parameter on the next line broke weirdly, e.g.

    , "//result/doc[2][str[@name='id'][.='p3s2']]" // 100 (boosted so treated as own group)
    , <more code>

becomes

            "//result/doc[2][str[@name='id'][.='p3s2']]" // 100 (boosted so treated as own
            // group)
            ,
            <more  code>

I suspect that changing the original to:
"//result/doc[2][str[@name='id'][.='p3s2']]", // 100 (boosted so treated as own group)

would fix it.

Searching these files: for a space-comma-EOL will show them: " ,$" if you think it's worth it. Actually, searching in IntelliJ will bring them up more easily...

Here's the command I used to find the files below:
find . -name "*.java" | xargs grep " ,$"

Didn't find any in the solrj code using the above. Nit-picky FWIW.

./src/test/org/apache/solr/handler/component/FacetPivotSmallTest.java:
./src/test/org/apache/solr/handler/component/StatsComponentTest.java:
./src/test/org/apache/solr/handler/component/QueryElevationComponentTest.java:
./src/test/org/apache/solr/handler/component/PhrasesIdentificationComponentTest.java:
./src/test/org/apache/solr/response/transform/TestSubQueryTransformer.java:
./src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java:
./src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java:
./src/test/org/apache/solr/search/TestExtendedDismaxParser.java:
./src/test/org/apache/solr/search/TestSearchPerf.java:
./src/test/org/apache/solr/search/TestSolrQueryParser.java:
./src/test/org/apache/solr/search/function/SortByFunctionTest.java:
./src/test/org/apache/solr/search/function/TestMinMaxOnMultiValuedField.java:
./src/test/org/apache/solr/search/TestQueryTypes.java:
./src/test/org/apache/solr/search/json/TestJsonRequest.java:
./src/test/org/apache/solr/search/TestCollapseQParserPlugin.java:
./src/test/org/apache/solr/search/TestBlockCollapse.java:
./src/test/org/apache/solr/search/TestPseudoReturnFields.java:
./src/test/org/apache/solr/search/TestTrieFacet.java:
./src/test/org/apache/solr/search/TestMissingGroups.java:
./src/test/org/apache/solr/search/TestSolr4Spatial.java:
./src/test/org/apache/solr/search/TestCustomSort.java:
./src/test/org/apache/solr/search/facet/TestJsonFacets.java:
./src/test/org/apache/solr/schema/TestSortableTextField.java:
./src/test/org/apache/solr/cloud/TestTolerantUpdateProcessorCloud.java:
./src/test/org/apache/solr/CursorPagingTest.java:
./src/test/org/apache/solr/request/SimpleFacetsTest.java:
./src/test/org/apache/solr/request/TestFaceting.java:
./src/test/org/apache/solr/TestGroupingSearch.java:

@risdenk
Copy link
Contributor Author

risdenk commented Mar 3, 2022

Thanks @ErickErickson! I am working through your comment fixes here: #720

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants