Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Document more TODOs as tickets #1418

Merged
merged 11 commits into from
Jan 30, 2019
Merged

Document more TODOs as tickets #1418

merged 11 commits into from
Jan 30, 2019

Conversation

gnunicorn
Copy link
Contributor

@gnunicorn gnunicorn commented Jan 14, 2019

Went through the TODOs, removed a bunch, which are outdated or nothing more than a regular comment, documented a bunch more as actual tickets and made them FIXMEs and unified their structure (FIXME #TICKETNO DESC for local tickets, FIXME: DESC LINK for external tickets) for easier in-editor support. Further more remove unnecessary remarks and related old code that I noticed in that instance.

@gnunicorn gnunicorn self-assigned this Jan 14, 2019
@gnunicorn gnunicorn added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 14, 2019
Copy link
Contributor Author

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

first batch of filing ToDos and FIXMEs as tickets.

Copy link
Contributor Author

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

next set processed.

@@ -21,7 +21,7 @@
//! The pool is able to return an iterator that traverses transaction
//! graph in the correct order taking into account priorities and dependencies.
//!
//! TODO [ToDr]
//! FIXME [ToDr]
//! - [ ] Multi-threading (getting ready transactions should not block the pool)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should remove these two lines.

@@ -94,7 +94,7 @@ impl trie_root::TrieStream for TrieStream {
_ => {
// println!("[append_substream] would have hashed, because data.len() = {}", data.len());
// data.encode_to(&mut self.buffer)
// TODO: re-enable hashing before merging
// FIXME: re-enable hashing before merging
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clearly old comments. let's remove

@gnunicorn gnunicorn requested a review from bkchr January 25, 2019 13:32
@gnunicorn gnunicorn added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 25, 2019
@gnunicorn gnunicorn added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). labels Jan 25, 2019
@gnunicorn gnunicorn changed the title Clean up TODO & FIXME comments Document more TODOs as tickets Jan 25, 2019
@gnunicorn gnunicorn added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 25, 2019
@@ -305,7 +305,6 @@ impl<B: ChainApi> Pool<B> {

impl<B: ChainApi> Pool<B> {
/// Create a new transaction pool.
/// TODO [ToDr] Options
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is actually still valid, logged here: #1579

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for logging, @tomusdrw but do you think this needs a code comment here as well? I think the ticket is more than fine enough :) .

Copy link
Contributor

@tomusdrw tomusdrw Jan 25, 2019

Choose a reason for hiding this comment

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

No, it's not needed. I'm actually tacking the issue right away anyway :)

Copy link
Member

@bkchr bkchr 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'm not sure if I'm happy with loosing clickable links to issues^^

@gnunicorn gnunicorn merged commit 4fa9fb6 into master Jan 30, 2019
@bkchr bkchr deleted the ben-cleanup-todos branch January 30, 2019 09:37
rphmeier pushed a commit that referenced this pull request Jan 30, 2019
Went through the TODOs, removed a bunch, which are outdated or nothing more than a regular comment, documented a bunch more as actual tickets and made them FIXMEs and unified their structure (`FIXME #TICKETNO DESC` for local tickets, `FIXME: DESC LINK` for external tickets) for easier in-editor support. Further more remove unnecessary remarks and related old code that I noticed in that instance.
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
Went through the TODOs, removed a bunch, which are outdated or nothing more than a regular comment, documented a bunch more as actual tickets and made them FIXMEs and unified their structure (`FIXME #TICKETNO DESC` for local tickets, `FIXME: DESC LINK` for external tickets) for easier in-editor support. Further more remove unnecessary remarks and related old code that I noticed in that instance.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants