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

CHIA-616 Annotate test_blockchain.py #18021

Merged
merged 1 commit into from
May 30, 2024
Merged

CHIA-616 Annotate test_blockchain.py #18021

merged 1 commit into from
May 30, 2024

Conversation

AmineKhaldi
Copy link
Contributor

No description provided.

@AmineKhaldi AmineKhaldi added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup labels May 15, 2024
@AmineKhaldi AmineKhaldi self-assigned this May 15, 2024
arvidn
arvidn reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

the type annotations look good. In some places you removed types (that mypy can deduce) which seems a little bit unnecessary (making the PR bigger).
I think you should restore all the trailing commas you removed, to make the PR smaller. If you feel strongly about formatting these a certain way, I think you should push for a policy (and maybe there's even tooling support to enforce it in black or flake8). By just sneaking in formatting changes like this, chances are that the next PR will work towards a different formatting goal and we'll just flip flop.

chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
@AmineKhaldi AmineKhaldi force-pushed the annotate_test_blockchain branch 2 times, most recently from 179dfb0 to 65338a5 Compare May 22, 2024 11:44
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label May 22, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@AmineKhaldi AmineKhaldi changed the title Annotate test_blockchain.py CHIA-616 Annotate test_blockchain.py May 28, 2024
arvidn
arvidn previously approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

I think it would be better to separate formatting changes into separate commits (and also justify them)

@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label May 29, 2024
@AmineKhaldi AmineKhaldi marked this pull request as ready for review May 30, 2024 09:29
@AmineKhaldi AmineKhaldi requested a review from a team as a code owner May 30, 2024 09:29
@cmmarslender cmmarslender merged commit a473d50 into Chia-Network:main May 30, 2024
354 of 355 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arvidn arvidn arvidn approved these changes

Assignees

@AmineKhaldi AmineKhaldi

Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AmineKhaldi @arvidn @cmmarslender