-
Notifications
You must be signed in to change notification settings - Fork 11
stricter checking of types #131
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
Conversation
043a004
to
ecf2e6a
Compare
9863289
to
39f9f21
Compare
the consequence of this is that we need to declare the non-existent function destroy in `Resource`. As a consequence, we have to explicitly declare it in the slightly specialized classes that directly inherit from it, `DefaultResource` and `ListableResource`. We "implement" it by raising an error. This patch fixes #156, and has been once again discovered by `mypy --strict` (#131).
the consequence of this is that we need to declare the non-existent function destroy in `Resource`. As a consequence, we have to explicitly declare it in the slightly specialized classes that directly inherit from it, `DefaultResource` and `ListableResource`. We "implement" it by raising an error. This patch fixes #156, and has been once again discovered by `mypy --strict` (#131).
39f9f21
to
b9a5886
Compare
issues (in mypy) discovered:
possible candidates: for errors such as:
in a function definition like: def cli(ctx,
force: bool=False,
release: bool=False,
recursive: bool=False,
download: bool=False,
filters: iocage.lib.Filter.Terms=None) -> None:
|
Don't you have to specify that? Like such
|
yup, according to the Python Zen, explicit is better than implicit. As such, mypy will be removing implicit |
14af576
to
da3d615
Compare
d1d42e5
to
4c3768e
Compare
89101ce
to
a6e6508
Compare
1dab5e5
to
9a17bca
Compare
e0a4389
to
f4eea17
Compare
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.
some questions
@@ -142,6 +144,7 @@ def get_command(self, ctx, name): | |||
pass | |||
return mod.cli | |||
except (ImportError, AttributeError): | |||
raise | |||
return |
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.
what?
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.
I need to add a --develop flag or something similar. This swallows code errors, which is okay for runtime, but hell for development. This is why it always re-appears here until I rebase a branch for merging.
@@ -75,3 +100,59 @@ def cli(ctx, rc, log_level, force, jails): | |||
jails_input = " ".join(list(jails)) | |||
logger.error(f"No jails matched your input: {jails_input}") | |||
exit(1) | |||
|
|||
|
|||
def normal( |
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.
what's normal anyway?
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.
not autostart
@@ -118,7 +118,7 @@ class JailConfigDefaults(iocage.lib.Config.Jail.BaseConfig.BaseConfig): | |||
|
|||
def __init__( | |||
self, | |||
logger: 'iocage.lib.Logger.Logger'=None | |||
logger: typing.Optional['iocage.lib.Logger.Logger']=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.
why is this a forward declaration?
@@ -104,17 +100,17 @@ class Fstab(list, iocage.lib.Config.Jail.File.Prototype.ResourceConfigFile): | |||
""" | |||
AUTO_COMMENT_IDENTIFIER = "iocage-auto" | |||
|
|||
release: 'iocage.lib.Release.ReleaseGenerator' | |||
release: typing.Optional['iocage.lib.Release.ReleaseGenerator'] |
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.
why is this a forward declaration?
host: 'iocage.lib.Host.HostGenerator'=None | ||
release: typing.Optional['iocage.lib.Release.ReleaseGenerator']=None, | ||
logger: typing.Optional['iocage.lib.Logger.Logger']=None, | ||
host: typing.Optional['iocage.lib.Host.HostGenerator']=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.
why is this a forward declaration?
def _update_freebsd_jail( | ||
self, | ||
jail: 'iocage.lib.Jail.JailGenerator' | ||
) -> typing.Generator['iocage.lib.events.IocageEvent', None, 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.
why is this a forward declaration?
self, | ||
update: typing.Optional[bool]=None, | ||
fetch_updates: typing.Optional[bool]=None | ||
) -> typing.List['iocage.lib.events.IocageEvent']: |
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.
why is this a forward declaration?
|
||
def update( # noqa: T484 | ||
self | ||
) -> typing.List['iocage.lib.events.IocageEvent']: |
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.
why is this a forward declaration?
safe_mode: bool=True, | ||
logger: 'iocage.lib.Logger.Logger'=None | ||
logger: typing.Optional['iocage.lib.Logger.Logger']=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.
why is this a forward declaration?
def clone_release(self, release): | ||
def clone_release( | ||
self, | ||
release: 'iocage.lib.Release.ReleaseGenerator' |
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.
why is this a forward declaration?
@gronke could you also explain the |
rather than implicitly setting a parameter `Optional[foo]`, by simply assigning it `None`, we make it explicit. This will be encouraged in newer mypy releases, and is currently enforced through `--strict`, so let's get going!
12bae38
to
9328f82
Compare
This patchset addresses #39.
we've now added the entire (default) config of flake8-mypy.
The following options deviate from their default (which gets us a step
closer to mypy --strct)
We've also toggled these two:
Finally, we check for
For the full --strict set, we'd also need:
But that, we'll need better typedefs.