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

[LANG-1547] fix code smells; fix typos #533

Closed
wants to merge 36 commits into from

Conversation

XenoAmess
Copy link
Contributor

No description provided.

@XenoAmess
Copy link
Contributor Author

XenoAmess commented May 30, 2020

Hi.
I implemented a lastIndexOf in the last commit.
Maybe we should open another pr for it but...anyway.
if it is acceptable, then I will make the indexOf using same way.
if not, then I will rebase it out.

@XenoAmess XenoAmess changed the title fix code smells; fix typos; performance refine(trying to) [LANG-1547] fix code smells; fix typos; performance refine(trying to) May 30, 2020
@jochenw
Copy link
Contributor

jochenw commented May 30, 2020

I'd find it reasonable to consider your changes in batches of, say, ten apiece. (That would make it possible to carefully inspect one by one.) But that many in one go? Definitely no.

@XenoAmess
Copy link
Contributor Author

XenoAmess commented May 31, 2020

I'd find it reasonable to consider your changes in batches of, say, ten apiece. (That would make it possible to carefully inspect one by one.) But that many in one go? Definitely no.

@jochenw
do you mean I should split them into several prs?
but I'm afraid that will cause pr spam and thus email spam. (as I'm somehow already considered pr spam recently).
anyway, I can move the last two commits into another pr as they are for performance and are too large.
------
done.
Is it considerable now?

@XenoAmess XenoAmess changed the title [LANG-1547] fix code smells; fix typos; performance refine(trying to) [LANG-1547] fix code smells; fix typos May 31, 2020
@XenoAmess
Copy link
Contributor Author

the performance refines I said has being splited to [LANG-1548] and [LANG-1549]

…fix_code_smells

� Conflicts:
�	src/test/java/org/apache/commons/lang3/LocksTest.java
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 95.036% when pulling 62f744a on XenoAmess:fix_code_smells into 3d4ed4a on apache:master.

@kinow
Copy link
Member

kinow commented Jun 1, 2020

I'd find it reasonable to consider your changes in batches of, say, ten apiece. (That would make it possible to carefully inspect one by one.) But that many in one go? Definitely no.

@XenoAmess it's quite hard to review this PR. It has several commits, and several changes. Committers will have to read and understand your changes, and then be confident that these changes can go in the next release without causing regressions.

As @jochenw pointed out, instead if you could prepare one pull request for a change, and either include your other changes in another PR, or wait for a review/merge to submit more, that would be much easier to review; at the same time, your PR would be reviewed much faster.

Bruno

@XenoAmess
Copy link
Contributor Author

I'd find it reasonable to consider your changes in batches of, say, ten apiece. (That would make it possible to carefully inspect one by one.) But that many in one go? Definitely no.

@XenoAmess it's quite hard to review this PR. It has several commits, and several changes. Committers will have to read and understand your changes, and then be confident that these changes can go in the next release without causing regressions.

As @jochenw pointed out, instead if you could prepare one pull request for a change, and either include your other changes in another PR, or wait for a review/merge to submit more, that would be much easier to review; at the same time, your PR would be reviewed much faster.

Bruno

OK, you're right, then I will split it into more prs.

@XenoAmess
Copy link
Contributor Author

@jochenw @kinow
Hi.
This pr is already splited into several prs.
Most of them are small enough that not need to have a jira (IMO).
So I will close this pr.
Thanks for your help.

@XenoAmess XenoAmess closed this Jun 1, 2020
@XenoAmess XenoAmess deleted the fix_code_smells branch June 16, 2020 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants