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 enumerations from stats_enums.hpp that are not used #4307

Merged

Conversation

RickiNano
Copy link
Contributor

Clean up enumerations that are no longer in use. This pr targets #4102

@gr0vity-dev
Copy link
Contributor

I'm wondering if it makes sense to have enums that are only used in the testcases ?
Do they make sense in a testcase when the codebase doesn't use them ?

Looking at the vote_processed enum, it got removed with the Optimistic elections PR in these commits :
e1c893b7dcbcfd9cf53eb1b0239e9febc62bb508
62811054380185c376df600501d22101b389d975

@clemahieu
Copy link
Contributor

clemahieu commented Oct 3, 2023

I'm wondering if it makes sense to have enums that are only used in the testcases ? Do they make sense in a testcase when the codebase doesn't use them ?

Looking at the vote_processed enum, it got removed with the Optimistic elections PR in these commits : e1c893b7dcbcfd9cf53eb1b0239e9febc62bb508 62811054380185c376df600501d22101b389d975

That is a good point and really I agree. It's occurred to me that we shouldn't be checking counters in tests at all. The stat counters are for users, and just as we shouldn't be depending on the output of logs, we shouldn't be depending on the counters.

@RickiNano
Copy link
Contributor Author

Do you want me to remove the enums again, and also remove the unit tests they are used in?

@clemahieu
Copy link
Contributor

No, let's do that separately. This change is fine on its own to remove enums that are completely unused.

@clemahieu clemahieu merged commit 1084814 into nanocurrency:develop Oct 3, 2023
16 of 17 checks passed
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.

3 participants