-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement typings in conda-smithy #1
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 9419614194Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
@@ -66,6 +66,10 @@ jobs: | |||
env: | |||
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |||
|
|||
- name: run type check | |||
run: | | |||
mypy conda_smithy tests |
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.
We only check the types at tests?
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 added type check for the folders 'conda_smithy' and 'tests'. wasn't sure if other files/folders require type check to run as well..
appveyor: bool = True, | ||
github_actions: bool = True, | ||
token_name: str = "BINSTAR_TOKEN", | ||
drone_endpoints = (), |
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.
Question: this is a type that doesn't exist? can't we at least use list
or function? (what is this?)
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.
it seems like list | tuple
worked. Only adding list
was throwing error that ()
is not a list
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.
My best guess is that ()
is the empty tuple, which is not a very useful value to pass to any function.
conda_smithy/cli.py
Outdated
subcommand = None | ||
aliases = [] | ||
subcommand: Optional[str] = None | ||
aliases: list = [] |
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.
Do we know the type of the list here so it can be List
?
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.
added List[str]
conda_smithy/configure_feedstock.py
Outdated
@@ -208,7 +260,7 @@ def break_up_top_level_values(top_level_keys, squished_variants): | |||
zip_key_groups = squished_variants["zip_keys"] | |||
if zip_key_groups and not isinstance(zip_key_groups[0], list): | |||
zip_key_groups = [zip_key_groups] | |||
zipped_configs = [] | |||
zipped_configs: list = [] | |||
top_level_dimensions = [] |
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 about the top_level_dimensions
?
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.
oops i missed that! added.
for idx, variant_key in enumerate(squished_variants[key]): | ||
top_level_config = [] | ||
top_level_config: Any = [] |
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 am sure you checked this, but can we avoid Any
?
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 is a tricky one:( top_level_config
is being declared as a list
initially and then being converted to tuple
on line 287. i tried using list | tuple
but then it throws another error for append
. If I leave out the type altogether this append
error still persists:(
Item "tuple[Any, ...]" of "list[Any] | tuple[Any, ...]" has no attribute "append"
I added Any
to avoid the errors since i couldn't find any other appropriate type 🙈
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.
If something is being converted to a different type, you could try using a new variable name when the conversion happens and giving each variable a distinct type.
conda_smithy/configure_feedstock.py
Outdated
top_level_loop_vars = set() | ||
|
||
all_used_vars = set() | ||
all_variants = set() | ||
all_variants: Any = set() | ||
|
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.
top_level_loop_vars and all_used_vars are already set?
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.
for some reason mypy was complaining that all_variants
needs a type. So I added Any
but now I updated it and added type as set
. Also updated the function _merge_deployment_target
on line 506 to set
for the types to align.
conda_smithy/configure_feedstock.py
Outdated
check=False, | ||
temporary_directory=None, | ||
forge_file_directory: str, | ||
forge_yml: 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.
Is this always 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.
oops no, i updated it to be Optional[str]
to have both str and None. (I use Optional
instead of |
because |
is causing error for python 3.8.19, which is what github actions is running..)
|
||
|
||
def get_repo(path, search_parent_directories=True): | ||
from io import TextIOWrapper |
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.
Please check this import
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.
now I saw it. Mh... I am unsure
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 was a suggestion when I ran monkeytype, but if it doesn't make sense we can take it out..
@@ -95,7 +95,7 @@ class BotConfigVersionUpdatesSourcesChoice(StrEnum): | |||
class AzureRunnerSettings(BaseModel): | |||
"""This is the settings for runners.""" | |||
|
|||
model_config: ConfigDict = ConfigDict(extra="allow") | |||
model_config = ConfigDict(extra="allow") |
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 did you remove the type?
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 error keeps showing up for ConfigDict. wasn't sure how to address this 🙈
Cannot override class variable (previously declared on base class "BaseModel") with instance variable
tests/test_feedstock_io.py
Outdated
@@ -11,17 +11,23 @@ | |||
|
|||
import git | |||
from git.index.typ import BlobFilter | |||
from git.repo.base import Repo |
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.
do we need this import?
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.
removed it.
@@ -5,6 +5,9 @@ | |||
|
|||
import pytest | |||
import scrypt | |||
from _pytest._py.path import LocalPath |
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.
Question: What is the difference between pytest
and _pytest
?
conda_smithy/configure_feedstock.py
Outdated
OrderedDict, | ||
List[Union[str, List[str]]], | ||
List[List[str]], | ||
], |
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 am not familiar with mypy specifically, so this is a hunch based on other type systems: I would expect the type List[Union[str, List[str]]]
to be identical to Union[List[str], List[List[str]]]
, so can this whole type be reduced to Union[List[Union[str, List[str]]], OrderedDict]
?
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.
Or, if the nested unions are too confusing, Union[List[str], List[List[str]], OrderedDict]
?
@@ -107,7 +136,7 @@ def package_key(config, used_loop_vars, subdir): | |||
return key.replace("*", "_").replace(" ", "_") | |||
|
|||
|
|||
def _ignore_match(ignore, rel): | |||
def _ignore_match(ignore: Union[Set[str], Tuple[()]], rel: str): |
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.
Did we find out what the type ()
refers to? I can't find it in the mypy docs, and Tuple[()]
(i.e. a tuple with a single element of type ()
) doesn't seem very useful.
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.
Ah ok, Tuple[()]
is the type for the empty tuple; if you use ()
as a type mypy will tell you to use Tuple[()]
or None
instead.
return sorted(range(len(seq)), key=seq.__getitem__) | ||
|
||
|
||
def sort_config(config, zip_key_groups): | ||
def sort_config( | ||
config: OrderedDict, zip_key_groups: List[Union[List[str], Any]] |
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.
Is the type Union[T, Any]
equivalent to Any
? If not, what makes it different and how are you allowed to use the value that might be a T
or anything else?
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.
adding the discussion from slack here
https://neighbourhoodie.slack.com/archives/C069J3L39HN/p1718277724884569?thread_ts=1717776546.052819&cid=C069J3L39HN
if x: Union[A, B, C...], then for the call x.foo(), all types A, B, C etc need to have a foo() method
so that means Union[T, Any] and Any are not the same thing; it means a value could be anything but you need to wrap it in isinstance(val, T) before using it
and then you get the benefits of type-checking T, whereas if you'd just used Any you'd get no type checks at all
OrderedDict, | ||
List[Union[str, List[str]]], | ||
List[List[str]], | ||
], |
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.
See earlier comment about reducing the complexity of this type.
conda_smithy/configure_feedstock.py
Outdated
Tuple[str, str, str, str], | ||
Tuple[str], | ||
Tuple[str, str, str, str, str], | ||
List[List[str]], |
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.
These tuples look odd -- this really looks like the function really expects an arbitrary-size collection of strings (List[str]
, Set[str]
etc) but some callers are passing this as a tuple. Rather than specifying every size of tuple we accept I would treat this as a signal to go and fix the callers to stop them using tuples.
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 changed it to this Union[set, dict, list, tuple]
keeping it generic.. not sure if this helps to reduce complexity..
List[Union[str, List[str]]], | ||
List[List[str]], | ||
], | ||
], |
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.
c.f. earlier comment to simplify this type.
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.
simplified it to generic Union[list, OrderedDict]
conda_smithy/configure_feedstock.py
Outdated
str, | ||
Union[ | ||
List[str], OrderedDict, List[Union[str, List[str]]], List[List[str]] | ||
], |
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.
Again this is equivalent to Union[List[str], List[List[str]], OrderedDict]
.
Union[List[str], Dict[str, Dict[str, str]], List[List[str]]], | ||
], | ||
] | ||
] = 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.
I think this whole type simplifies to Optional[Dict[str, Union[List[str], List[List[str]], Dict[str, Dict[str, str]]]]]
.
], | ||
], | ||
Dict[str, Union[str, bool]], | ||
], |
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 believe this type is equivalent to:
Union[
CommentedMap,
Dict[str, bool],
Dict[str, str],
Dict[str, List[str]],
Dict[str, Dict[str, bool]],
Dict[str, Dict[str, Dict[str, str]]]
]
Or, if you want to remove a bit of duplication without too much extra union nesting:
Union[
CommentedMap,
Dict[str, Union[
bool,
str,
List[str],
Dict[str, bool],
Dict[str, Dict[str, str]]
]]
]
I think either of these is more readable than the existing type.
Pull Request Test Coverage Report for Build 9587371809Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
tests/test_cli.py
Outdated
@@ -14,12 +14,12 @@ | |||
|
|||
_thisdir = os.path.abspath(os.path.dirname(__file__)) | |||
|
|||
InitArgs = collections.namedtuple( | |||
InitArgs = collections.namedtuple( # type: ignore | |||
"ArgsObject", |
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 think you can resolve this by passing "InitArgs"
as the first argument instead of "ArgsObject"
, if I understood this correctly. Makes no difference to the tests, at least.
Same for the one below, pass in "RegenerateArgs" instead.
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 was afraid this might break the test so i ignored it but it worked! updated it.
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.
Sorry, I meant to just make comments, not start a review 🙈
conda_smithy/schema.py
Outdated
@@ -12,7 +12,7 @@ | |||
from conda.base.constants import KNOWN_SUBDIRS | |||
|
|||
try: | |||
from enum import StrEnum | |||
from enum import StrEnum # type: ignore | |||
except ImportError: | |||
from backports.strenum import StrEnum |
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 think it's better to ignore the error on the second import, at least that’s what I gather from here.
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.
hmm moving the ignore to the second import StrEnum brings back the error: Module "enum" has no attribute "StrEnum"
:(
Pull Request Test Coverage Report for Build 9596910568Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
27ea642
to
e3ef37d
Compare
Pull Request Test Coverage Report for Build 9599544458Details
💛 - Coveralls |
86fcf8b
to
ce9dc1f
Compare
Pull Request Test Coverage Report for Build 9599610108Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9599947730Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9702412366Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
82d1fb1
to
b2a66d4
Compare
Pull Request Test Coverage Report for Build 9702463236Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9702496514Details
💛 - Coveralls |
00b9c49
to
a4e89f7
Compare
Pull Request Test Coverage Report for Build 10599090712Details
💛 - Coveralls |
Add mypy to GitHub workflow and to the environment yml
Add mypy type ignore for some tests and conda smithy files, which couldn't be resolved.
47bcea6
to
bafe408
Compare
Introduction
This work is done according to "Milestone 2: Implement typings in conda-smithy" as stated in the Scope of Work with Sovereign Tech Fund (STF) (https://www.sovereigntechfund.de/).
This PR addresses the typing request from this issue
Setup
Setup details are here: https://github.com/neighbourhoodie/conda-smithy/wiki
Steps followed to add type
As suggested in the ticket in the conda-smithy repo, MonkeyType is used to generate type hints for the python code.
Generate type hints
monkeytype.sqlite3
in the current working directory.stubs
to save the type hint filesThe above command will create stub files in the
stubs
folder. Here's an example of what the content of a stub file will look like. It gives suggestions for types.Checklist