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.

Sign up for GitHub

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

Jump to bottom

Installed some internal generics to fix some compiler warnings #5576

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Installed some internal generics to fix some compiler warnings #5576

merged 1 commit into from
Nov 7, 2023

Conversation

tbw777
Copy link
Contributor

@tbw777 tbw777 commented Feb 28, 2023

Only private simple non generic collections was refactored to generics if no out of class usage detected.
#5524

@mbien mbien added Code cleanup ci:all-tests [ci] enable all tests labels Feb 28, 2023
@apache apache locked and limited conversation to collaborators Feb 28, 2023
@apache apache unlocked this conversation Feb 28, 2023
@tbw777
Copy link
Contributor Author

tbw777 commented Mar 2, 2023 •
edited
Loading

Cant push another PR while this waiting something. Are NB is alive?
@mbien
@BradWalker

@tbw777
Copy link
Contributor Author

tbw777 commented Mar 4, 2023

inconsistent, corrected

@tbw777 tbw777 closed this Mar 4, 2023
@tbw777 tbw777 reopened this Mar 4, 2023
@vieiro
Copy link
Contributor

vieiro commented Mar 4, 2023

Hi, please focus in #5598 before posting new PRs.
Once that one is ready for review you can then reopen this one.
Thanks.

@vieiro vieiro closed this Mar 4, 2023
@tbw777
Copy link
Contributor Author

tbw777 commented May 15, 2023

@vieiro The PR is ready to be reopen

@vieiro vieiro reopened this May 16, 2023
@tbw777
Copy link
Contributor Author

tbw777 commented Jun 5, 2023 •
edited
Loading

@mbien
@BradWalker

The PR is actual

@tbw777
Copy link
Contributor Author

tbw777 commented Oct 15, 2023 •
edited
Loading

@mbien
@BradWalker
@vieiro

@tbw777
Copy link
Contributor Author

tbw777 commented Nov 2, 2023

up

@BradWalker
Copy link
Member

Overall, I'm always in support of code cleanup.

I would suggest that in the future you try and keep the changes in the same domain. This will make code review easier.

For example..
1 - If you say I'm going to clean up module XYZ, then it's appropriate to make a bunch of different changes in there.
2 - If you say I'm going to clean up the List generic, then keep the changes to cleaning up JUST the List interface.

It becomes harder for a volunteer like me to do a code review when a bunch of unrelated changes are requested. Doesn't mean the changes aren't useful, it's just harder to read/understand.

@tbw777
Copy link
Contributor Author

tbw777 commented Nov 6, 2023

@BradWalker accepted

@mbien mbien added this to the NB21 milestone Nov 6, 2023
@BradWalker BradWalker merged commit 0c8313e into apache:master Nov 7, 2023
42 of 43 checks passed
@tbw777 tbw777 mentioned this pull request Nov 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BradWalker BradWalker BradWalker approved these changes

@mbien mbien mbien approved these changes

Assignees

@tbw777 tbw777

Labels
ci:all-tests [ci] enable all tests Code cleanup
Projects
None yet
Milestone
NB21
Development

Successfully merging this pull request may close these issues.
4 participants
@tbw777 @vieiro @BradWalker @mbien