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

Clean up ledger::process return value. #4442

Merged

Conversation

clemahieu
Copy link
Contributor

@clemahieu clemahieu commented Feb 19, 2024

Remove process_return class which only contained process_result.
Rename process_result to block_status to better indicate its purpose.

@RickiNano
Copy link
Contributor

I'm new to the codebase so it may just be me who is confused, but I find the name ledger_code to be confusing and not descriptive of what it is. The word "code" is very generic and can be so many things.
How about ledger_result? Or ledger_status?
Or if code needs to be in the name, then ledger_result_code or ledger_status_code

@clemahieu
Copy link
Contributor Author

I think a longer name here is overly descriptive. The main c++ type to report errors is std::error_code so I think nano::ledger_code matches this.

I would have made this a nested enum of ledger nano::ledger::code, though nested types cannot be forward declared so I named it nano::ledger_code.

This is some external discussion on overly long names: https://wiki.c2.com/?VeryLongDescriptiveNamesThatProgrammingPairsThinkProvideGoodDescriptions

@RickiNano
Copy link
Contributor

I was not advocating for longer naming per se, but just replacing "code" with "status" or "result" that has more meaning (at least to me :) )

…me process_result to ledger_code to better indicate its purpose.
@clemahieu clemahieu force-pushed the process_result_enum_rename branch from 920f92f to 0278131 Compare February 20, 2024 17:01
@clemahieu
Copy link
Contributor Author

Changed to nano::block_status

@clemahieu clemahieu merged commit 4030663 into nanocurrency:develop Feb 20, 2024
24 of 27 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.

2 participants