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

remove verify_account_history_plugin_index() #915

Merged
merged 1 commit into from
Aug 17, 2018

Conversation

oxarbitrage
Copy link
Member

function verify_account_history_plugin_index() in the test cases do return immediately after it is called, ignoring the body on it fully. haves no sense to keep or use IMO(maybe i am missing something), this PR remove function and calls to it.

@oxarbitrage oxarbitrage added code cleanup 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists labels May 11, 2018
@abitmore
Copy link
Member

I think we need to fix it rather than removing it. Maybe add similar tests for other plugins.

@ryanRfox
Copy link
Contributor

I believe this is a Protocol Impacting Issue. If so, may I request @oxarbitrage @abitmore to Label it and place it on the proper Release (remove the incorrect one, as I assigned it to both).

@oxarbitrage
Copy link
Member Author

not protocol update here. what happens is that the code in question is not active in the moment so i removed it. @abitmore think the code should be fixed instead of removed. I didn't had the time to take a look on how hard to fix it yet however this is something we probably can do in the next release.

@ryanRfox
Copy link
Contributor

@oxarbitrage Thanks for clarifying the Release placement. May I request you add the reference to the Issue number? I am unable to locate an Issue this fixes.

@oxarbitrage
Copy link
Member Author

there is no issue for this, just the pull at the moment.

@cogutvalera
Copy link
Member

maybe we should create issue for this PR ?

Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

AFAICS the commented-out code checks some mapping of accounts to addresses.
That doesn't make sense at all, so I don't see how this could be fixed, therefore I approve removal.
(Feel free to disagree ofc.)

@abitmore
Copy link
Member

I agree. The last related commit is this: a0765e2, which removed a lot of code but left some commented out. According to changes and the commit comment, the commented out code doesn't make sense any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists code cleanup testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants