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

Renaming RestartTasks to ConsensusTxsTask - Merged on a closed PR #1167

Merged
merged 8 commits into from
Oct 19, 2019

Conversation

vncoelho
Copy link
Member

This also helps for understanding #1148

@erikzhang, please consider this renaming. RestartTasks was a name hard to understand.
In fact, that class was just used at ConsensusService, which forces a restart of global and knowhashes

I believe that comprehension will improve with this.

@vncoelho vncoelho changed the title Renaming restart tasks Renaming RestartTasks to ConsensusTxsTask Oct 18, 2019
@codecov-io
Copy link

Codecov Report

Merging #1167 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1167   +/-   ##
=======================================
  Coverage   65.14%   65.14%           
=======================================
  Files         199      199           
  Lines       13598    13598           
=======================================
  Hits         8858     8858           
  Misses       4740     4740
Impacted Files Coverage Δ
neo/Consensus/ConsensusService.cs 15.3% <0%> (ø) ⬆️
neo/Network/P2P/TaskManager.cs 15.64% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4112df2...4d5d214. Read the comment docs.

@erikzhang
Copy link
Member

erikzhang commented Oct 19, 2019

It is not related to consensus.

@shargon
Copy link
Member

shargon commented Oct 19, 2019

Why you don't group it with #1162, two PR for renaming seems weird to me

@vncoelho
Copy link
Member Author

vncoelho commented Oct 19, 2019

They came in separate idea, @shargon, can be merged there, but @erikzhang is saying it is not related to consensus. However, it is just used there nowadays.

I prefer to rename and if, in the future, we have other applications we create the tasks for them.

@vncoelho vncoelho changed the base branch from master to refactoring-transaction-verify October 19, 2019 12:24
@vncoelho vncoelho merged commit 05014da into refactoring-transaction-verify Oct 19, 2019
@vncoelho vncoelho deleted the renaming-restart-tasks branch October 19, 2019 12:24
@vncoelho
Copy link
Member Author

merged on #1162

@vncoelho vncoelho restored the renaming-restart-tasks branch October 23, 2019 17:27
@shargon shargon deleted the renaming-restart-tasks branch October 23, 2019 17:30
@vncoelho
Copy link
Member Author

@shargon, maybe I will reopen this branch in another PR.

@vncoelho vncoelho restored the renaming-restart-tasks branch October 23, 2019 17:44
@vncoelho vncoelho changed the title Renaming RestartTasks to ConsensusTxsTask Renaming RestartTasks to ConsensusTxsTask - Merged on a closed PR Oct 23, 2019
@shargon shargon mentioned this pull request Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants