ecmwf / pyflow

A high level Python interface to ecFlow allowing the creation of ecFlow suites in a modular and "pythonic" way
https://pyflow-workflow-generator.readthedocs.io/en/latest/
Apache License 2.0
7 stars 7 forks source link

Change kwargs type to str #8

Closed kinow closed 1 year ago

kinow commented 1 year ago

I think kwargs is a dict, but either my IDE and/or mypy recognize it already as a dict, and in the pydoc comments we can put just the value of the type stored in it (my guess & from this mypy page).

Here's what it looks like whenever I use an extra variable in a family:

Screenshot from 2022-11-29 16-39-17

Hovering the mouse, the warning shown says “Expected type 'dict', got 'str' instead”. After this change:

Screenshot from 2022-11-29 16-39-49

I was going over other places where kwargs is used, but I found one where it was actually using an object that extends Script. I was inclined to use Any there, but preferred to first raise this PR/issue, and see what others prefer :+1:

Thanks! -Bruno

corentincarton commented 1 year ago

Hi @kinow, it's a small detail but it makes sense. We shouldn't do it only for the Family class though. Could you push a version that fixes all the Nodes families? Thanks for this!

kinow commented 1 year ago

Hi @kinow, it's a small detail but it makes sense. We shouldn't do it only for the Family class though. Could you push a version that fixes all the Nodes families? Thanks for this!

Sure thing! Thanks @corentincarton !

kinow commented 1 year ago

I agree that dict is wrong according to PEP 484. We need to make sure that str isn't too restrictive in some cases, or rather, make sure all non-variable kwargs are documented separately.

I will try to add it to the other parts of the code, and try to run mypy too (PyCharm already integrates it, but I will run separately) and report here. Then we can do another :eyes: review again to see if it's looking OK.

FussyDuck commented 1 year ago

CLA assistant check
All committers have signed the CLA.

kinow commented 1 year ago

Done. Updated all nodes to use **kwargs(str) type. Had a look at the code, and couldn't find any problem. Also tried:

(pyflow) kinow@ranma:~/Development/python/workspace/pyflow$ pip install mypy
Collecting mypy
  Downloading mypy-0.991-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (18.2 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 18.2/18.2 MB 2.3 MB/s eta 0:00:00
Collecting typing-extensions>=3.10
  Using cached typing_extensions-4.4.0-py3-none-any.whl (26 kB)
Requirement already satisfied: tomli>=1.1.0 in /home/kinow/mambaforge/envs/pyflow/lib/python3.10/site-packages (from mypy) (2.0.1)
Collecting mypy-extensions>=0.4.3
  Using cached mypy_extensions-0.4.3-py2.py3-none-any.whl (4.5 kB)
Installing collected packages: mypy-extensions, typing-extensions, mypy
Successfully installed mypy-0.991 mypy-extensions-0.4.3 typing-extensions-4.4.0
(pyflow) kinow@ranma:~/Development/python/workspace/pyflow$ mypy pyflow/nodes.py 
Success: no issues found in 1 source file

Although not sure if we need mypy.ini or some other configuration in order for MyPy to perform more tests (i.e. no idea how good is its default parameters).

Cheers

kinow commented 1 year ago

Thank you @corentincarton , @oiffrig !