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.

Sign up for GitHub

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

Jump to bottom

SOLR-16070: Enable spotless for solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/ #717

Merged
merged 10 commits into from Apr 26, 2022
Merged

SOLR-16070: Enable spotless for solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/ #717

merged 10 commits into from Apr 26, 2022

Conversation

risdenk
Copy link
Contributor

@risdenk risdenk commented Mar 2, 2022

@risdenk risdenk self-assigned this Mar 2, 2022
@risdenk
Copy link
Contributor Author

risdenk commented Mar 2, 2022

@ErickErickson you can push to my fork/branch if you want to update it that way.

This shows the 13 files changed with ~7700 added ~5300 removed. If you look at the files you will see why I skipped these. The fixes aren't straightforward :(

risdenk
risdenk commented Mar 2, 2022
View reviewed changes
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.

Reviewed the 8/13 files under 1000 lines.

.../solrj/src/test/org/apache/solr/client/solrj/io/stream/ParallelFacetStreamOverAliasTest.java Outdated Show resolved Hide resolved
.../solrj/src/test/org/apache/solr/client/solrj/io/stream/ParallelFacetStreamOverAliasTest.java Outdated Show resolved Hide resolved
.../solrj/src/test/org/apache/solr/client/solrj/io/stream/ParallelFacetStreamOverAliasTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionToExpessionTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionToExpessionTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionToExpessionTest.java Outdated Show resolved Hide resolved
...solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionToExplanationTest.java Outdated Show resolved Hide resolved
...solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionToExplanationTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/JDBCStreamTest.java Outdated Show resolved Hide resolved
risdenk
risdenk commented Mar 2, 2022
View reviewed changes
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.

Reviewed CloudAuthStreamTest.java

solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java Outdated Show resolved Hide resolved
risdenk
risdenk commented Mar 2, 2022
View reviewed changes
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.

Reviewed streaming test

solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Outdated Show resolved Hide resolved
risdenk
risdenk commented Mar 2, 2022
View reviewed changes
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.

The following 3 files haven't been reviewed yet:

  • MathExpressionTest
  • StreamDecoratorTest
  • StreamExpressionTest

Yes they are the 2-3k lines changed files :)

@dsmiley
Copy link
Contributor

dsmiley commented Mar 28, 2022

Shall we just merge this (subject to merge conflict resolution) and move on?

@risdenk
Copy link
Contributor Author

risdenk commented Apr 5, 2022

I'll come back to this. It fell off my radar with work stuff. I'll make sure we get this in.

@risdenk risdenk marked this pull request as ready for review April 25, 2022 20:58
@risdenk
Copy link
Contributor Author

risdenk commented Apr 25, 2022

Finally got back to this and it looks ready to go. Thanks @ErickErickson for a lot of cleanup! I fixed up the conflicts and some other minor changes.

dsmiley
dsmiley approved these changes Apr 26, 2022
View reviewed changes
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 but I barely looked at this. It's just to some tests. I think we should merge this ASAP and get it into 9.0 RC4 as it's the last of the formatting changes and such changes are intrinsically super safe. No big deal any way, to be honest.

CC @janhoy

@risdenk
Copy link
Contributor Author

risdenk commented Apr 26, 2022

I'll get this into main and branch_9x shortly. @janhoy let me know if branch_9_0 is a problem or not.

@janhoy
Copy link
Contributor

janhoy commented Apr 26, 2022

Currently trying to build RC4, so please target for 9_x for now.

@risdenk risdenk merged commit b218c17 into apache:main Apr 26, 2022
@risdenk risdenk deleted the SOLR-16070 branch April 26, 2022 15:10
risdenk added a commit that referenced this pull request Apr 26, 2022
@risdenk
SOLR-16070: Enable spotless for solr/solrj/src/test/org/apache/solr/c…
8edaf18 
…lient/solrj/io/stream/ (#717)

* SOLR-16070: Enable spotless for solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/

Co-authored-by: Erick Erickson <Erick.Erickson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsmiley dsmiley dsmiley approved these changes

@ErickErickson ErickErickson Awaiting requested review from ErickErickson

@thelabdude thelabdude Awaiting requested review from thelabdude

@madrob madrob Awaiting requested review from madrob

@HoustonPutman HoustonPutman Awaiting requested review from HoustonPutman

@cpoerschke cpoerschke Awaiting requested review from cpoerschke

@epugh epugh Awaiting requested review from epugh

@gerlowskija gerlowskija Awaiting requested review from gerlowskija

@janhoy janhoy Awaiting requested review from janhoy

Assignees

@risdenk risdenk

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@risdenk @dsmiley @janhoy