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.

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

SOLR-10458: setFollowRedirects should be deprecated in favor of Solr Client Builder methods #1216

Merged
merged 5 commits into from Dec 8, 2022

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Dec 6, 2022

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

Description

Use the builder pattern for following redirects.

Solution

update test. Found a bug in the HttpSolrClient use of the builder pattern for this property!

Tests

ran

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@epugh epugh requested review from risdenk and dsmiley December 6, 2022 23:20
@epugh epugh merged commit 46bce23 into apache:main Dec 8, 2022
epugh added a commit that referenced this pull request Dec 8, 2022
…Client Builder methods (#1216)

Fix followRedirect property on HttpSolrClient not set when using Builder pattern.  Migrated tests to use Builder.
@dsmiley
Copy link
Contributor

dsmiley commented Dec 8, 2022

for a boolean, arguably naming the method followRedirects would be fine without the "with" up front. Note other booleans like "compression" have a setter builder method: allowCompression and there are probably others. I think the point is for it to read nicer. The "with" isn't wrong but removing it is just as clear, thus why have a superfluous word.
FWIW @anshumg added the Builders long ago from what I see.

@dsmiley
Copy link
Contributor

dsmiley commented Dec 12, 2022

ping @epugh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants