#10036 Replace codename Merge with Bellatrix (2nd step)#10116
Conversation
leolara
changed the title
leolara
force-pushed
the
refactor/I10036/rename-merge-to-bellatrix-2
branch
from
79b8885 to
e94e66f
Compare
|
@terencechain after rebase there is a protobuf compilation error, but bazel hides the underlaying error |
leolara
force-pushed
the
refactor/I10036/rename-merge-to-bellatrix-2
branch
2 times, most recently
from
01f9e54 to
d7be947
Compare
|
@terencechain please check |
|
After this have been approved I think I can fix the rest in another PR |
leolara
force-pushed
the
refactor/I10036/rename-merge-to-bellatrix-2
branch
from
d7be947 to
505183a
Compare
potuz
left a comment
potuz
left a comment
There was a problem hiding this comment.
For other reviewers (if others are deemed necessary) here are some sanity checks that I have passed through this PR:
1- git diff develop --name-only | xargs cat | grep Merge to catch for missed merge names in the changed files. Only few instances are found, only verbs.
2. Same with "merge".
3. Checked all instances of changes in the diff to see if they made sense. (without actually checking for code logic)
I left comments throughout the PR requesting to change "bellatrix" for "Bellatrix". These are optionals. I know there's a rule about error messages being lower case, but to my own taste proper names should be capitalized. The same applies to Altair which is not the scope of this PR, and others may disagree, so I'll approve right now and leave those comments there in case someone wants to change them.
leolara
force-pushed
the
refactor/I10036/rename-merge-to-bellatrix-2
branch
from
113cc01 to
59eb089
Compare
|
This seems ready to merge!! LGTM |
| // UpgradeToBellatrix updates inputs a generic state to return the version Bellatrix state. | ||
| // It inserts an empty `ExecutionPayloadHeader` into the state. | ||
| func UpgradeToBellatrix(ctx context.Context, state state.BeaconState) (state.BeaconState, error) { | ||
| func UpgradeToBellatrix(_ context.Context, state state.BeaconState) (state.BeaconState, error) { |
There was a problem hiding this comment.
Are we sure about this change? It hides an error that is unrelated to this PR and may make it harder to debug later if the function is not properly implemented. The parameter should go, or just keep the error. Incidentally. Each time you push you make stale my previous approval. The PR does look good to me
There was a problem hiding this comment.
@potuz I don't have an opinion about this, I just did it so "Deepsource" wouldn't protest. If we can merge it with the "Deepsource" error, I can revert it, no problem.
Are you suggesting to remove the parameter all the way? Or merging ignoring "Deepsource"?
There was a problem hiding this comment.
I would suggest to leave the error as is since this is not the scope of the PR, we can wait until Monday and let @terencechain chime in, since he'll write the PR implementing the upgrade, but either way, hiding the error i think shouldn't be the best option
There was a problem hiding this comment.
+1
Let's leave the error as it is
leolara
force-pushed
the
refactor/I10036/rename-merge-to-bellatrix-2
branch
from
a2d0808 to
59eb089
Compare
|
@terencechain removed the error correction. Let me know if we need anything else to merge this? :-) |
rkapka
left a comment
rkapka
left a comment
There was a problem hiding this comment.
Approving changes to state package.
leolara
force-pushed
the
refactor/I10036/rename-merge-to-bellatrix-2
branch
from
59eb089 to
8c44466
Compare
|
I just rebased to make in sync with develop. Are we merging this? @rkapka @terencechain @potuz I think it does not let me even if approved because it does not pass the "deepsource: go" step |
It's fine the Deepsource error, we can merge with that. The problem is that each time you push changes, you make the previous approval stale. There's been like 10 pushes since I last approved this, will need to review them. I suppose the same applies to @rkapka's review |
|
Sorry, I will not rebase anymore. I didn't know it would cause trouble. I don't like to have many "merge develop" into the history, so I usually do that. But I will not do this here anymore |
Everything gets squashed into 1 commit when we merge into |
|
Let's wait for approval from @terencechain or @potuz before merging. |
potuz
left a comment
potuz
left a comment
There was a problem hiding this comment.
As far as I can tell these changes are fine
|
@potuz it does not automerge because of the deepsource error. What should we do? |
|
@leolara I just started the CI build for your PR, it should merge soon if there are any issues. Deepsource isn't a requirement for a successful merge of the PR. |
|
Thanks @nisdas I am just starting to learn your process :-) |
5 participants
What type of PR is this?
Refactoring
What does this PR do? Why is it needed?
Replace codename Merge with Bellatrix, focusing on proto package
Which issues(s) does this PR fix?
Partially #10036
Other notes for review
I had to manually fix three files generated from protobuf, for some reason ./hack/update-go-pbs.sh adds a silly syntax error to these files