fix mypy errors: component.py #549
Conversation
stevebachmeier
requested review from
albrja,
collijk,
hussain-jafari,
patricktnast and
rmudambi
as code owners
| @@ -92,7 +92,24 @@ class Component(ABC): | |||
| component. An empty dictionary indicates no managed configurations. | |||
| """ | |||
|
|
|||
| def __repr__(self): | |||
| def __init__(self) -> None: | |||
There was a problem hiding this comment.
This is a directly copy/paste that moves the constructor to the top. mypy really didn't like the init being at the bottom of the file.
| @@ -791,7 +798,7 @@ def _register_simulant_initializer(self, builder: Builder) -> None: | |||
|
|
|||
| if type(self).on_initialize_simulants != Component.on_initialize_simulants: | |||
| builder.population.initializes_simulants( | |||
| self, creates_columns=self.columns_created, **initialization_requirements | |||
| self, creates_columns=self.columns_created, **initialization_requirements # type: ignore[arg-type] | |||
There was a problem hiding this comment.
This block is temporary anyway since we're deprecating old functionalty here. It does highlight the fact, however, that we need to figure out how to best type hint kwargs that get unpacked at the signature. the current solution seems complicated. https://typing.readthedocs.io/en/latest/spec/callables.html#unpack-kwargs
| else: | ||
| try: | ||
| data = builder.data.load(data_source) | ||
| except ArtifactException: | ||
| raise ConfigurationError( | ||
| f"Failed to find key '{data_source}' in artifact." | ||
| ) | ||
| elif isinstance(data_source, Callable): | ||
| elif callable(data_source): |
There was a problem hiding this comment.
til about the built-in callable and how isinstance(func, collections.abc.Callable) isn't proper
| """ | ||
| return None | ||
| return "" |
There was a problem hiding this comment.
This seems like a big change based on validation for empty strings vs None. What is the reason for this change or was the type hinting just wrong?
There was a problem hiding this comment.
Still working on this (the PR is in draft)
There was a problem hiding this comment.
I think it's fine, though. It did indeed get flagged by mypy but all of the usage of the query are handling strings
There was a problem hiding this comment.
I guess there's a chance that some downstream code is assuming this could be None. I can handle it if we think that's actually the case. but vph isn't using this property at all and everything in vivarium handles it as a str. (all of the defaults, e.g. are "", not None)
| @@ -606,7 +607,9 @@ def build_lookup_table( | |||
| raise ConfigurationError(f"Data '{data}' must be a LookupTableData instance.") | |||
|
|
|||
| if isinstance(data, list): | |||
| return builder.lookup.build_table(data, value_columns=list(value_columns)) | |||
| return builder.lookup.build_table( | |||
| data, value_columns=list(value_columns) if value_columns else () | |||
There was a problem hiding this comment.
Shouldn't this pass an empty list if value columns is None?
There was a problem hiding this comment.
doesn't really batter b/c build_table takes a Sequence (in fact it defaults to ())
src/vivarium/component.py
Outdated
| all_columns = list(data.columns) | ||
| if value_columns is None: | ||
| if not self.get_value_columns: |
There was a problem hiding this comment.
self.get_value_columns will never actually be None at this point, since it's being set earlier in setup_component(). I think it's better to ignore the mypy error than introduce this check that will never trigger.
| @@ -38,7 +37,7 @@ def _terminal_logging_not_configured() -> bool: | |||
| # fragile since it depends on a loguru's internals as well as the stability of code | |||
| # paths in vivarium, but both are quite stable at this point, so I think it's pretty, | |||
| # low risk. | |||
| return 1 not in logger._core.handlers # type: ignore[attr-defined] | |||
| return 1 not in loguru.logger._core.handlers # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
Why was this change needed?
There was a problem hiding this comment.
Just a generic cleanup (we're already importing all of loguru). And to make it consistent w/ te component.py
stevebachmeier
deleted the
sbachmei/mic-5599/fix-mypy-errors-component
branch
Fix mypy errors: component.py
Description
Changes and notes
Testing