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-16903: Update CLI tools to use java.nio.file.Path instead of java.io.File #2883

Merged
merged 3 commits into from
Nov 26, 2024

Conversation

AndreyBozhko
Copy link
Contributor

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

Description

I would like to work on SOLR-16903 - this PR only addresses part of it.

Here, I'm migrating the CLI tools from using java.io.File to java.nio.file.Path APIs.

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, not available for branches on forks living under an organisation)
  • 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

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

LGTM. One tiny nit pick... I love to see these refactorings happening!

@@ -281,15 +281,15 @@ public void testAppendUrlPath() {
@Test
public void testGuessType() {
File f = new File("foo.doc");
Copy link
Contributor

Choose a reason for hiding this comment

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

total nit pick, but would declaring the File f as a Path p save a .toPath???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - but I thought it was best to update the test files separately and at a later point.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome!

@epugh
Copy link
Contributor

epugh commented Nov 26, 2024

@AndreyBozhko let me know when you feel the PR is ready...!

@AndreyBozhko
Copy link
Contributor Author

@epugh Thanks, I think this piece is ready.

@epugh
Copy link
Contributor

epugh commented Nov 26, 2024

Since this is purely a refactoring, skipping CHANGES.txt. I am going to apply this to main only unless folks think we should be backporting these refactorings?

@epugh epugh merged commit 6d167b1 into apache:main Nov 26, 2024
4 checks passed
@epugh
Copy link
Contributor

epugh commented Nov 26, 2024

@AndreyBozhko please do tag me as you keep doing these! I also like to keep an eye on https://ci-builds.apache.org/job/Solr/job/Solr-Test-main/ to make sure things remain good as these changes are made!

@AndreyBozhko AndreyBozhko deleted the solr-16903-pt-1 branch November 26, 2024 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants