lucene
lucene copied to clipboard
Convert more classes to record classes
Description
- This PR addresses #13207 to convert more classes on
mainbranch 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
TotalHitsandSynonymMapas 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.
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.
I addressed your comments in the new revision and all the checks are passing now after your fix. Thanks!
Thanks, looks fine. I have no time to do another closer review, please give me some time to proceed.
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!
Hi @shubhamvishu, can you check my comments? The changes here look fine, please fix the remaining issues. Uwe
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.
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!
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!
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?
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!
Added the blocker label to draw attention to this PR for the Lucene 10 release.
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!
@jpountz I addressed your comments and added a CHANGES, MIGRATE entry now. Thanks for reviewing!
I think we've run into a conflict with #13689. @jpountz - should I fix that or are you already doing it?
@javanna just fixed it, thanks @javanna !