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

style: avoid repetion in Transaction variant names #222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tdelabro
Copy link

@tdelabro tdelabro commented Jul 31, 2024

fixes: starkware-libs/blockifier#1706

This change is Reviewable

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)


crates/blockifier/src/transaction/transaction_execution.rs line 66 at r1 (raw file):

                    false => DeclareTransaction::new(declare, tx_hash, non_optional_class_info),
                };
                Ok(Self::AccountTransaction(AccountTransaction::Declare(declare_tx?)))

I think that Self::Account might be a bit confusing here. Account and AccountTransaction are different things. I fear that a reader unfamiliar with the code might think we are creating an account that will run the transaction.

Code quote:

(Self::AccountTransaction(AccountTransaction::Declare(declare_tx?))

@tdelabro
Copy link
Author

Due to the way Enum syntax works there won't be any occurrence of Account just by itself, it will always be Account::Transaction which is self-explanatory and doesn't remove information in comparison to Account::AccountTransaction.

When using Self::Transaction you are always in a context where you are fully aware of what Self is.

So I don't think it is misleading at all. Especially because it is coherent with the rest of the naming convention used

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tdelabro)

Copy link

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction enum's variant name are too verbose
2 participants