Improve collection related things that reusing a immutable object instead of creating a new object#4135
Improve collection related things that reusing a immutable object instead of creating a new object#4135gianm merged 1 commit intoapache:masterfrom asdf2014:code_refactoring
collection related things that reusing a immutable object instead of creating a new object#4135Conversation
There was a problem hiding this comment.
Is it fully compatible? Couldn't somebody depend on the actual name of the module somehow?
There was a problem hiding this comment.
I think just we can ensure the name of SimpleModule be unique, that will be okay.
There was a problem hiding this comment.
Last two )) should be on the next line
There was a problem hiding this comment.
Could be new byte[] {value.byteValue()}
There was a problem hiding this comment.
I don't think it should be renamed
There was a problem hiding this comment.
U r right. I'll roll it back.
|
|
|
LGTM. Thanks @asdf2014! |
|
@asdf2014 could you please resolve conflicts? Then we can merge this PR. |
|
@asdf2014 your latest commit doesn't have druid-io's master HEAD commit as parent, so Github doesn't hide repetitive changes. Could you please add parent to your latest commit (http://stackoverflow.com/a/4264151/648955 might help) and re-push it? |
|
In the future please resolve conflicts via |
|
@leventov Hi, Leventov. I executed the |
|
If that is fine. I would push it with the |
…nstead of creating a new object
|
@leventov Already Done. git rebase --abort
git checkout code_refactoring
git checkout -b re2
git branch root bbb61e638b391d29
git checkout re2
git diff master > ../123.patch
git checkout master
git checkout -b r2
git apply ../123.patch
git diff master
git status
git diff --name-status | wc -l
git add .
git status
git diff master
git commit -m 'Improve `collection` related things that reusing a immutable object instead of creating a new object'
git status
git push origin r2:code_refactoring -f |
|
Why i changed nothing, but i got the Can anyone help me to restart the build job? |
|
@asdf2014 looks like the tests have passed now. thanks for the contribution! |
| return reader.read(null, DecoderFactory.get().binaryDecoder(inputStream, null)); | ||
| } | ||
| catch (EOFException eof) { | ||
| } catch (EOFException eof) { |
There was a problem hiding this comment.
Actually this violate Druid code style. Former version of code is correct
|
@asdf2014 irrelevant now but actually what you did is rebase of your entire branch, that is also should be avoided. I asked to rebase only the latest commit in the branch, which you was proposing to merge into druid-io/druid/master, |
|
@asdf2014 for the future PRs - please don't squash anything after you submitted a PR and some time has passed so somebody may has already started looking at your PR (and, moreover, if somebody already left some review comments), because reviewers lose the progress and in some cases need to re-review the whole PR, to be sure that they didn't miss anything. |
|
Okay, you are right. Those messages of commits should be retained. |
|
In addition to the messages, Github also has a nice feature that lets reviewers see what has changed since their last review. It's based on diffing against new commits though, so squashing prevents that feature from working. FWIW we only very recently changed our guidelines to suggest that people avoid squashing (#4087). Until then we did suggest squashing, but we changed our minds :) |
|
Oh... Don't know these, i will pay attention in the future. |
|
No worries, and thanks again for the patch. |
|
Alright, you are welcome. |
4 participants
Improve
collectionrelated things that reusing a immutable object instead of creating a new objectCollections.singletonList(something)is immutable whereasArrays.asList(something)is a fixed size List representation of an Array where the List and Array gets joined in the heap.Collections.emptyList()reuses an object instead of creating a new object as it will be the case withArrays.asList().