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

Delete outdated TODOs refering to closed issues #6732

Merged
merged 24 commits into from
May 23, 2023
Merged

Delete outdated TODOs refering to closed issues #6732

merged 24 commits into from
May 23, 2023

Conversation

mpguerra
Copy link
Contributor

@mpguerra mpguerra commented May 19, 2023

Motivation

We want to clean up any code comments relating to TODO tasks that have already been addressed.

Specifications

Complex Code or Requirements

Solution

Deletes comments related to issues that have been closed as fixed.

Depends-On: #6737

Review

For each deleted comment the reviewer should double check that the PR really addresses the issue which is mentioned in the code comment.

Informational comments that refer to closed issues should not be removed unless they are no longer relevant.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

This is just a first pass. There are still a number of issues that have been closed as wont-fix referenced in the code and we should decide what we do about those.

Depends-

@mpguerra mpguerra added A-docs Area: Documentation C-cleanup Category: This is a cleanup C-audit Category: Issues arising from audit findings labels May 19, 2023
@mpguerra mpguerra marked this pull request as ready for review May 19, 2023 13:28
@mpguerra mpguerra requested review from a team as code owners May 19, 2023 13:28
@mpguerra mpguerra requested review from teor2345 and removed request for a team May 19, 2023 13:28
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #6732 (32d37c7) into main (f2be848) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6732      +/-   ##
==========================================
- Coverage   77.98%   77.98%   -0.01%     
==========================================
  Files         308      308              
  Lines       40869    40872       +3     
==========================================
- Hits        31873    31872       -1     
- Misses       8996     9000       +4     

@teor2345 teor2345 removed the request for review from a team May 21, 2023 19:05
@teor2345 teor2345 removed the request for review from a team May 21, 2023 19:05
teor2345
teor2345 previously approved these changes May 21, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, but I think we can update some of these comments based on how their tickets were resolved.

zebra-chain/src/work/difficulty/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/limit.rs Show resolved Hide resolved
zebra-network/src/policies.rs Show resolved Hide resolved
zebra-network/src/protocol/external/codec.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
Co-authored-by: teor <teor@riseup.net>
mpguerra and others added 5 commits May 22, 2023 10:05
Co-authored-by: teor <teor@riseup.net>
…ownloads

Co-authored-by: teor <teor@riseup.net>
…o fix

Co-authored-by: teor <teor@riseup.net>
…o change in future

Co-authored-by: teor <teor@riseup.net>
Co-authored-by: teor <teor@riseup.net>
@gustavovalverde
Copy link
Member

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing my fixes, I fixed the rustfmt and the merge conflict, both caused by my PRs and suggestions.

@mergify mergify bot merged commit ec2e9ca into main May 23, 2023
@mergify mergify bot deleted the pili-6281 branch May 23, 2023 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation C-audit Category: Issues arising from audit findings C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants