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

Categorize methods in RichIterable using region/endregion comments. #1689

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

donraab
Copy link
Contributor

@donraab donraab commented Sep 1, 2024

  • Updated top of RichIterable javadoc to include method categories with links to methods
  • Created region/endregion sections to correspond to method categories

@donraab
Copy link
Contributor Author

donraab commented Sep 7, 2024

@motlin @prathasirisha Anything you need me to do to help land this PR? Thanks!

@donraab
Copy link
Contributor Author

donraab commented Sep 8, 2024

@motlin @prathasirisha I did a quick sanity check and compared method counts between 11.1 and 12.0 with this PR (there is a difference of two new methods in 12.0 for reduceBy. If this isn't enough, I will see if I can do a more detailed method signature diff to make this easier to land.

RichIterable 11.1

Type No-arg Methods Lambda methods Lazy Methods Eager Methods Return Collection Return Void Return Primitive Unique Names Total Methods
RichIterable 30 133 3 181 100 8 32 134 184

RichIterable 12.0

Type No-arg Methods Lambda methods Lazy Methods Eager Methods Return Collection Return Void Return Primitive Unique Names Total Methods
RichIterable 30 135 3 183 102 8 32 135 186

@donraab
Copy link
Contributor Author

donraab commented Sep 10, 2024

@motlin @prathasirisha If there are no comments in the next two days, I will merge this PR. The only issue I see for this PR is that it will cause merge conflicts for PRs with any changes to RichIterable that are already in progress. The diff is difficult to read as methods were moved around, and the only concern there should be that I accidentally deleted something or changed a method body. If I changed a method body, then tests should break. If I deleted something, the diff tables I provided should look different. I diff'd with EC 11.1 so I could also see how much RichIterable has changed since 11.1. We've only added two methods, both reduceBy variations. I expect this to mean there are likely no substantive changes to RichIterable currently in progress.

@donraab
Copy link
Contributor Author

donraab commented Sep 10, 2024

@motlin @prathasirisha Here's a gist that shows how I have organized methods into method categories.

If you're looking for where methods have moved, this might be helpful. The categories here should match the order they appear in the file.

This blog also has an image of the Javadoc for RichIterable generated from this PR.

@motlin
Copy link
Contributor

motlin commented Sep 11, 2024

I haven't had a chance to review this, but I don't have any particular concerns about sorting methods other than the potential for merge conflicts.

I looked through my open PRs, and I don't think I'm going to get merge conflicts in any of mine. However, I have 10 non-draft ones that have passed tests that have been open for a while, and I'd love to land mine as well. I have other work that's waiting on my earlier work to land.

I haven't checked anyone else's PRs for merge conflicts. Of our 42 open PRs, 23 of them are non-draft, have all passed checks, and have not been reviewed yet, so 13 more that aren't mine. I'd like to get into a faster pace of landing open PRs, but for this one I'd at least like to make sure it doesn't introduce merge conflicts.

@donraab
Copy link
Contributor Author

donraab commented Sep 11, 2024

@motlin This PR is one class. Even if anyone has changed RichIterable in another PR, it can only be one of three things.

  1. Adding a new method (easy to copy/paste).
  2. Adding a new default implementation for an existing method (easy to copy/paste).
  3. Changing an existing default implementation of an existing method (easy to copy/paste).

No one should be doing a lot of work on anything on RichIterable (other than me reorganizing) as this interface is solid. Any issues about changing this interface in the wild are likely one or two method additions (e.g. groupByEachUniqueKey).

Merge conflicts have a very low percentage chance of causing any actual pain with this type IMO. Any merge conflicts may be noisy, but can be easily fixed if someone remembers what they were working on in a PR and simply reapplies it in the right place in the new version of the type.

My two cents.

@donraab
Copy link
Contributor Author

donraab commented Sep 14, 2024

@motlin @prathasirisha The only open PR with a potential merge conflict with RichIterable is #1353, which has been open for a while and has requested changes from Craig. To merge all that would be needed is copying the containsAll and containsAllIterable changes to the new file.

I am merging this now. Thanks.

@donraab donraab merged commit eea0abd into eclipse:master Sep 14, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants