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

Fixes+silences type errors in models #155

Merged
merged 22 commits into from
Mar 11, 2021
Merged

Fixes+silences type errors in models #155

merged 22 commits into from
Mar 11, 2021

Conversation

tedkalaw
Copy link
Contributor

@tedkalaw tedkalaw commented Mar 11, 2021

High Level Overview of Change

Some were fixable, others were faster to silence. Comments inline

After this, there are 3 type errors left in all of xrpl!

Context of Change

Needed so we can use mypy on xrpl

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release
  • Typing

Before / After

mypy returns only 3 errors in xrpl

Test Plan

CI
poetry run mypy --strict xrpl
3 errors should be returned:

xrpl/models/base_model.py:34: error: Too many arguments for "BaseModel"
xrpl/models/currencies/currency.py:11: error: Argument 2 to NewType(...) must be subclassable (got "Union[XRP, IssuedCurrency]")
xrpl/models/amounts/amount.py:10: error: Argument 2 to NewType(...) must be subclassable (got "Union[str, IssuedCurrencyAmount]")

@tedkalaw tedkalaw changed the title Models types Fixes+silences type errors in models Mar 11, 2021
@@ -17,4 +17,4 @@ class IssuedCurrencyAmount(IssuedCurrency):
See https://xrpl.org/currency-formats.html#issued-currency-amounts.
"""

value: str = REQUIRED
value: str = REQUIRED # type: ignore
Copy link
Contributor Author

@tedkalaw tedkalaw Mar 11, 2021

Choose a reason for hiding this comment

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

this causes a type error because the type of value is a str, but the default value is REQUIRED (an object). i tried a couple patterns but ran into the issue that the generated getter has a type of Union[<intended type>, object] which means that at call sites, you have to narrow the type...and we don't want to expose this to callers anyhow

this will likely require a deeper refactor - i could imagine this being implemented as a list of required attributes or a few other ways, although my understanding is that this is a tricky part of dataclass subclassing. as such, i opted for # type: ignore, as this preserves the type of the getter. not ideal, but don't think i'll have the time to go deeper so i did this everywhere in models

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually i think this is the optimal solution. value = REQUIRED should be a type error in all cases because if a value = REQUIRED then an error will be thrown

@@ -80,28 +80,28 @@ def _get_errors(self: PathStep) -> Dict[str, str]:

def _get_account_error(self: PathStep) -> Optional[str]:
if self.account is None:
return
return None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for Optional return types, mypy by default requires you to return None instead of having an empty return. this seems like a stylistic choice to me, but opted to just make it pass the default settings to simplify configuration

if we want, we can enable a flag (warn_no_return) in the config to make this work

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine w/ this

@tedkalaw tedkalaw marked this pull request as ready for review March 11, 2021 00:45
@mvadari
Copy link
Collaborator

mvadari commented Mar 11, 2021

You will probs need to do more once #131 is merged in

@tedkalaw
Copy link
Contributor Author

sure thing!

@tedkalaw tedkalaw merged commit d8f7ad2 into master Mar 11, 2021
@tedkalaw tedkalaw deleted the models-types branch March 11, 2021 18:44
@mDuo13 mDuo13 added this to the First pip release milestone Mar 24, 2021
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.

4 participants