lucene icon indicating copy to clipboard operation
lucene copied to clipboard

Convert more classes to record classes

Open shubhamvishu opened this issue 1 year ago • 8 comments

Description

  • This PR addresses #13207 to convert more classes on main branch to record classes on main (Lucene 10 only).
  • It moves a lot of data classes(120 to be precise) to use record classes that were flagged suitable in my IDE and seemed good candidate in general.
  • I left a couple of them like TotalHits and SynonymMap as the PR is already very big and including those were leading to a lot more changes. Maybe we could do those as a separate PR.

  Raising this PR since all the tests are passing(./gradlew test) but renderJavadoc task is complaining about missing java docs on some record classes(converted in this PR) which I see has the javadocs already eg: TermStats, ReaderSlice. I'm not sure why its flagging those incorrectly or if maybe I'm missing something.

shubhamvishu avatar Apr 30 '24 02:04 shubhamvishu

Hi, you can merge main into your branch and hopefully checks pass. For some packages like the "codecs" one, the new code requires you to also document all record components with @param.

uschindler avatar Apr 30 '24 16:04 uschindler

I addressed your comments in the new revision and all the checks are passing now after your fix. Thanks!

shubhamvishu avatar May 02 '24 00:05 shubhamvishu

Thanks, looks fine. I have no time to do another closer review, please give me some time to proceed.

uschindler avatar May 02 '24 21:05 uschindler

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

github-actions[bot] avatar May 17 '24 00:05 github-actions[bot]

Hi @shubhamvishu, can you check my comments? The changes here look fine, please fix the remaining issues. Uwe

uschindler avatar May 18 '24 17:05 uschindler

Hi @uschindler , sorry I was on vacation until last week, so this PR stalled. I'll take a look at the comments today or tomorrow.

Update : Got stuck with some other work. Will take a look at this sometime this week.

shubhamvishu avatar May 20 '24 17:05 shubhamvishu

Hi @uschindler , thanks for the review. I took another pass at the changes and pushed some commits addressing your comments. Please have a look when you get a chance. Thanks!

shubhamvishu avatar Jun 03 '24 00:06 shubhamvishu

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

github-actions[bot] avatar Jun 26 '24 00:06 github-actions[bot]

Hi @uschindler, Could you please review the current code changes once you get some time and if it looks good maybe we can move this forward?

shubhamvishu avatar Jul 04 '24 16:07 shubhamvishu

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

github-actions[bot] avatar Jul 19 '24 01:07 github-actions[bot]

Added the blocker label to draw attention to this PR for the Lucene 10 release.

stefanvodita avatar Sep 09 '24 15:09 stefanvodita

Hi @uschindler @mikemccand @jpountz , I fixed the recent merge conflicts and looking for reviews as 10.0 release is blocked on this change. This PR has been attracting merge conflicts frequently since its touching 200+ files, so hoping we could close this sooner. Let me know if there is any concerns here(I'm up for quick followups). Thanks!

shubhamvishu avatar Sep 09 '24 16:09 shubhamvishu

@jpountz I addressed your comments and added a CHANGES, MIGRATE entry now. Thanks for reviewing!

shubhamvishu avatar Sep 10 '24 09:09 shubhamvishu

I think we've run into a conflict with #13689. @jpountz - should I fix that or are you already doing it?

stefanvodita avatar Sep 10 '24 21:09 stefanvodita

@javanna just fixed it, thanks @javanna !

jpountz avatar Sep 10 '24 21:09 jpountz