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

Assign some methods as properties #19

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HarukaMa
Copy link
Contributor

@HarukaMa HarukaMa commented Oct 28, 2023

Draft: depends on #18

Basically, to make the SDK more pythonic. Some methods like account.address() could be written as account.address, as arguably address is a property of account. IMO we don't really need to keep the Rust semantics here.

My criteria is:

  • Returned value could be seen as a property of the object
  • No other inputs needed to compute the value (see RecordPlaintext.serial_number_string)
  • The method is reasonably fast to execute
  • The method has no side effects

As a side note, to_string methods feels superfluous as users should just use str(...) in Python to get a string representation of the object. I decided to not removing those here though.

@iamalwaysuncomfortable iamalwaysuncomfortable marked this pull request as ready for review November 7, 2023 13:10
@HarukaMa HarukaMa changed the base branch from fix/re-organize-python-account-methods to master December 8, 2023 14:44
@HarukaMa
Copy link
Contributor Author

HarukaMa commented Dec 8, 2023

need to review and reconsider if methods from new classes should be changed as well. draft for now.

@HarukaMa HarukaMa marked this pull request as draft December 8, 2023 14:45
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.

1 participant