Skip to content
This repository has been archived by the owner on Feb 9, 2023. It is now read-only.

Remove Outdated Assertions #109

Merged
merged 11 commits into from
Aug 10, 2022
Merged

Remove Outdated Assertions #109

merged 11 commits into from
Aug 10, 2022

Conversation

Pandapip1
Copy link
Member

@Pandapip1 Pandapip1 commented Aug 3, 2022

Blocking ethereum/EIPs#5400. It is safe to do this.

@Pandapip1 Pandapip1 changed the title Remove Outdated Assertion Remove Outdated Assertions Aug 3, 2022
@Pandapip1
Copy link
Member Author

@MicahZoltu I request your "submit review"-clicking prowess.

@MicahZoltu
Copy link
Contributor

Please get a review from another editor. I don't feel comfortable deciding what is/isn't appropriate to merge as I'm no longer an editor.

@Pandapip1
Copy link
Member Author

CC @JEAlfonsoP, but @MicahZoltu you are the only person that has the ability to merge.

Actually, I guess @axic does.

@MicahZoltu
Copy link
Contributor

I don't mind clicking the button once there is approval from other editors, but I don't want to be the one deciding to click the button.

Pandapip1 referenced this pull request Aug 8, 2022
@@ -33,5 +33,5 @@ inputs:
runs:
using: "composite"
steps:
- run: cd ${{github.action_path}} && yarn install && yarn build && ERC_EDITORS=${{ inputs.ERC_EDITORS }} CORE_EDITORS=${{ inputs.CORE_EDITORS }} NETWORKING_EDITORS=${{ inputs.NETWORKING_EDITORS }} INTERFACE_EDITORS=${{ inputs.INTERFACE_EDITORS }} META_EDITORS=${{ inputs.META_EDITORS }} INFORMATIONAL_EDITORS=${{ inputs.INFORMATIONAL_EDITORS }} GITHUB_TOKEN=${{ inputs.GITHUB-TOKEN }} MAINTAINERS=${{ inputs.MAINTAINERS }} NODE_ENV=production node build/src/index.js
- run: cd ${{github.action_path}} && yarn install && yarn build && ERC_EDITORS=${{ inputs.ERC_EDITORS }} CORE_EDITORS=${{ inputs.CORE_EDITORS }} NETWORKING_EDITORS=${{ inputs.NETWORKING_EDITORS }} INTERFACE_EDITORS=${{ inputs.INTERFACE_EDITORS }} META_EDITORS=${{ inputs.META_EDITORS }} INFORMATIONAL_EDITORS=${{ inputs.INFORMATIONAL_EDITORS }} GITHUB_TOKEN=${{ inputs.GITHUB-TOKEN }} MAINTAINERS=${{ inputs.MAINTAINERS }} PR_NUMBER=${{ inputs.PR_NUMBER }} NODE_ENV=production node build/src/index.js
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the multiline syntax for this?

Suggested change
- run: cd ${{github.action_path}} && yarn install && yarn build && ERC_EDITORS=${{ inputs.ERC_EDITORS }} CORE_EDITORS=${{ inputs.CORE_EDITORS }} NETWORKING_EDITORS=${{ inputs.NETWORKING_EDITORS }} INTERFACE_EDITORS=${{ inputs.INTERFACE_EDITORS }} META_EDITORS=${{ inputs.META_EDITORS }} INFORMATIONAL_EDITORS=${{ inputs.INFORMATIONAL_EDITORS }} GITHUB_TOKEN=${{ inputs.GITHUB-TOKEN }} MAINTAINERS=${{ inputs.MAINTAINERS }} PR_NUMBER=${{ inputs.PR_NUMBER }} NODE_ENV=production node build/src/index.js
- run: |
cd ${{github.action_path}} && \
yarn install && \
yarn build && \
ERC_EDITORS=${{ inputs.ERC_EDITORS }} \
CORE_EDITORS=${{ inputs.CORE_EDITORS }} \
NETWORKING_EDITORS=${{ inputs.NETWORKING_EDITORS }} \
INTERFACE_EDITORS=${{ inputs.INTERFACE_EDITORS }} \
META_EDITORS=${{ inputs.META_EDITORS }} \
INFORMATIONAL_EDITORS=${{ inputs.INFORMATIONAL_EDITORS }} \
GITHUB_TOKEN=${{ inputs.GITHUB-TOKEN }} \
MAINTAINERS=${{ inputs.MAINTAINERS }} \
PR_NUMBER=${{ inputs.PR_NUMBER }} \
NODE_ENV=production \
node build/src/index.js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, but that should be done in a separate PR. This is known to work and this change would require re-testing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're changing this code, so it's probably the best time to rework it.

Is testing it painful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is testing it painful?

Very. Takes about 30 minutes to do a full E2E test, and it has to be done manually by two people simultaneously.

Copy link

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, better to get some code working and improve it later. I already spend too much time on this stuff 🤣

@Pandapip1
Copy link
Member Author

@MicahZoltu

@MicahZoltu MicahZoltu merged commit 2521019 into ethereum:master Aug 10, 2022
@Pandapip1 Pandapip1 deleted the patch-5 branch August 10, 2022 13:52
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants