-
Notifications
You must be signed in to change notification settings - Fork 94
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
Changes from all commits
633cb35
c27fffb
2563648
d43ac31
c824ab0
ed29121
b357a35
8a97ba7
fa24a02
78f1b26
8dfdfd7
57be070
bfe0f8c
cff95a9
d287b5c
02cd773
ef58503
362e58c
a427106
aa51c39
740ef01
d7e04a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,28 +82,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for if we want, we can enable a flag ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine w/ this |
||
if self.currency is not None or self.issuer is not None: | ||
return "Cannot set account if currency or issuer are set" | ||
return | ||
return None | ||
|
||
def _get_currency_error(self: PathStep) -> Optional[str]: | ||
if self.currency is None: | ||
return | ||
return None | ||
if self.account is not None: | ||
return "Cannot set currency if account is set" | ||
if self.issuer is not None and self.currency.upper() == "XRP": | ||
return "Cannot set issuer if currency is XRP" | ||
return | ||
return None | ||
|
||
def _get_issuer_error(self: PathStep) -> Optional[str]: | ||
if self.issuer is None: | ||
return | ||
return None | ||
if self.account is not None: | ||
return "Cannot set issuer if account is set" | ||
if self.currency is not None and self.currency.upper() == "XRP": | ||
return "Cannot set issuer if currency is XRP" | ||
return | ||
return None | ||
|
||
|
||
@require_kwargs_on_init | ||
|
@@ -132,10 +132,10 @@ class PathFind(Request): | |
a symptom of heavy server load.) | ||
""" | ||
|
||
subcommand: PathFindSubcommand = REQUIRED | ||
source_account: str = REQUIRED | ||
destination_account: str = REQUIRED | ||
destination_amount: Amount = REQUIRED | ||
subcommand: PathFindSubcommand = REQUIRED # type: ignore | ||
source_account: str = REQUIRED # type: ignore | ||
destination_account: str = REQUIRED # type: ignore | ||
destination_amount: Amount = REQUIRED # type: ignore | ||
method: RequestMethod = field(default=RequestMethod.PATH_FIND, init=False) | ||
send_max: Optional[Amount] = None | ||
paths: Optional[List[List[PathStep]]] = None |
There was a problem hiding this comment.
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 astr
, but the default value isREQUIRED
(anobject
). i tried a couple patterns but ran into the issue that the generated getter has a type ofUnion[<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 anyhowthis 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 inmodels
There was a problem hiding this comment.
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