Type hint pr

[tychodub]

Adds type hints to all RobotMBT source files, as far as possible without updating from Python 3.10 to newer versions. The Python 3.10 constraint means that "Self" types have not had type hints added.

Ignore the large amounts of commits, since those are an artifact of the rebasing process.
Should be merged with a squash, so the large amount of commits don't show up.

[tychodub]

I pushed a commit with all the changes except for those that required further elaboration.

[JFoederer]

Great progress! Let's conclude the remaining open conversations and work towards the merge. Amazing how much insights this typing exercise brought up!

[tychodub]

The last commit message also mentions the changes that were made in commit a95c0ef, because I did not see said commit until I committed my local changes which contained the same changes.

[JFoederer]

Compatibility check with Robot framework 7.3.2 passed.

[JFoederer]

From the three enum vs. literal discussions I updated the argument kind to use enum. If we like this approach better, then the gherkin_kw is also a candidate for conversion. The ROBOT_LIBRARY_SCOPE is not our type, so that will not change in this pull request.

[JFoederer]

I did not opt to change gherkin_kw to enum. It did not help the readability of the code, due to the frequent checking of subsets and checking its boolean/None value in if gherkin_kw.

[JFoederer]

@tychodub, are there still open items on your list where you needed clarification to complete the type hints?

Other than that there is just one discussion still open. To conclude whether we accept a dict type to be a stub for ModelSpace and str for Scenario in TraceState, or that we create proper types for these stubs.

[tychodub]

@tychodub, are there still open items on your list where you needed clarification to complete the type hints?

Other than that there is just one discussion still open. To conclude whether we accept a dict type to be a stub for ModelSpace and str for Scenario in TraceState, or that we create proper types for these stubs.

I think there are no open items left.
For the discussion surrounding types that are only used in test: I personally think type hints should reflect intended usage of methods and not be influenced by unit tests.

1. If a test ends up going wrong because a wrong type gets used, it will be easier to find out the root cause if it is specifically indicated that said type is not truly supported.
2. Adding types that only get used for small tests (and are not truly supported by the method) may confuse end-users of the methods. Since type hints serve as a form of documentation, it is important that type hints can be trusted to be accurate.

[JFoederer]

Ok. I think we already agreed that no test-specific typing would be left in production code. There is still a couple to be removed then.

If I interpret your remark correctly, then you also propose to use proper typing the other way around. In which case we can add simple stubs like class Scenario(str): pass to the test file.

[tychodub]

Ok. I think we already agreed that no test-specific typing would be left in production code. There is still a couple to be removed then.

If I interpret your remark correctly, then you also propose to use proper typing the other way around. In which case we can add simple stubs like class Scenario(str): pass to the test file.

Is the second part of your message directed to @osingaatje? He suggested also fixing the types used in the unit tests. While I do think it's better for unit tests to be written with the correct types, I don't think updating the unit tests to do so is a priority right now. Also, Scenario can already be constructed with just a string, so no stub definition is necessary.

[JFoederer]

I updated utest\test_tracestate.py but didn't have the time to do utest\test_tracestate_refinement.py.
Also, my pylance does show the type hints, but does not complain when using a different type in a call. Can you check if the current solution is good enough to prevent the warnings you were getting?

[osingaatje]

Argument of type "Scenario" cannot be assigned to parameter "scenario" of type "Scenario" in function "confirm_full_scenario"
  "utest.test_tracestate.Scenario" is not assignable to "robotmbt.suitedata.Scenario"PylancereportArgumentType

We cannot use stubs here to get Pylance to shut up.

I suggest:

from robotmbt.suitedata import Scenario
from robotmbt.modelspace import ModelSpace

Because Scenario already has a string-only constructor no code changes are needed (except explicit Scenario(name=...), but for ModelSpace I'd suggest a _from_dict function, and then changing ModelSpace(a=3) to ModelSpace.from_dict({a:3}) (or using a private method in the utest itself)

[JFoederer]

How is this for e.g. TestSteps in test_suitedata.py? Where we patch the unit to import the stub instead of the real thing?

I do not want to import cross dependencies for a unit test, because that would make it an integration test.

[JFoederer]

I reproduced the error message after finding the relevant settings in pylance. Getting them fixed is another story. It expects type stubs in .pyi files, but that is about as far as I got today. I got distracted by all the other files it also marks red.

[JFoederer]

I will try to draw some conclusions and wrap up an (intermediate) solution tomorrow. Otherwise this merge will get too close to your final presentation.

[JFoederer]

Status on type hints

Included type hints for:

• arguments
• return types, except return None
• member variables

Excluded:

• local variables
• warnings on optional variables (... | None)
• stub/mock type warnings in test files
• Self type (pending dropping Python 3.10 support)