WIP: Add Typing Support (PEP 561 - issue #447)

[abravalheri]

The changes here try to address #447, but they would be more of a RFC than a PR...

I tried to add the most complete implementation that would support the use case described, however there might be room for simplification (currently it is very complex).

One of the things that I noticed is that this implementation highlights problems related to #336, and exposes an abstraction leak if we move in that direction: the project structure abstraction works very well for updating files, but the mental model is not easily applicable to updating parts of files or composing them incrementally with different extensions.

In the code provided we can see this abstraction leak by comparing the add_typing_support and update_existing_config actions: they are very similar. The fact that we need 2 different actions that are alternating most of times is also a symptom. The modify_file is basically doing the same as structure.modify_contents but instead of operating in the dict representation, it operates directly on the file system.

---

I followed the example listed in the PEP561 which places a src/{package}/py.typed file. In principle the namespace extension have the machinery to automatically convert the struct.

---

Simplifications we could do, and reasons why I haven't implemented that way (yet):

1. We could add a typecheck task to tox.ini by default, since running tox is opt-in.

   • In Support PEP 561 - Typing Support #447, it is mentioned that other extensions could add type dependencies (e.g. pyscaffoldext-django and https://github.com/typeddjango/django-stubs). The way I implemented it was by exposing a typecheck_deps option that can be augmented when extensions are activated. The logic to add these deps to tox.ini by default would be very similar to the one implemented and therefore I don't see many simplifications by making it a default behaviour.
   • The current implementation have the advantage of being able to update existing project.

2. We could add the mypy config to setup.cfg by default.

   • I have the impression that doing so have the potential to mess up the behaviour of some IDEs in the case the user does not want to use typing.
   • Some type dependencies (e.g. the django-stubs previously mentioned) require mypy to have some plugins configured, and therefore we would need to have some level of manipulation of setup.cfg anyway.
   • The current implementation have the advantage of being able to update existing project.

3. We could add type hints to skeleton.py by default.
   I have chosen to not do so because some Python beginners coming from other languages might feel overwhelmed by a type system.

---

Things that I dislike in the PR implementation:

• retype being an optional dependency that is required to run --type. The alternative would be either making Typed an external extension or making retype a default dependency...
• The files modified by retype do not pass the isort pre-commit.

Things that I like in the current implementation:

• structure.modify_contents seems to be a nice addition to the API.

[coveralls]

Pull Request Test Coverage Report for Build 6308941580730368

• 102 of 102 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 97.41%

---

Totals
Change from base Build 5626408933261312:	0.1%
Covered Lines:	1765
Relevant Lines:	1798

---

💛 - Coveralls

[FlorianWilhelm]

Thanks for the suggestions! Sorry for the late reply and I also only had a quick look at the changes. Right now got I lot of work on my plate as colleagues are on vacation.

I must say that it really looks a bit complicated and adds more complexity in the PyScaffold codebase for a feature I am not 100% convinced of. Option 3, you are mentioning, actually sounds like the best for me. This is for several reasons. Most Python programmers I work with are adopting types in their code directly. And with PyScaffold we are addressing clean code affine and thus more advanced developers anyway, not beginners that just start scripting. So for our target group seeing some types shouldn't be a problem and I strongly believe typed Python code will become the default in production-grade code. Since PyScaffold helps you to develop production-grade packages, why not have a template example that is typed by default? Option 3 would be simple, with no need for an extension and extra complexity in the code-base.

What do you think?

[abravalheri]

Thank you very much for the review @FlorianWilhelm, I implemented a few changes:

• Types are added by default to skeleton.py
• typecheck task added by default to tox.ini
• mypy config added by default to setup.cfg
• typecheck task added by default to .cirrus.yml, however it is disabled via an ENV var working as a flag (otherwise people that do not opt-in for --typed will have their build failing).

The remaining complexity is due to:

• a. The ability to "flip" the TYPE_CHECKING flag on .cirrus.yml on --type (and therefore enable the task)
• b. The ability to add typechecker config to tox.ini/setup.cfg in existing projects via --update
• c. The ability of having other extensions adding dependencies for the typecheck (django-stubs is mentioned in Support PEP 561 - Typing Support #447)
• d. The related ability of adding mypy plugins (that come with the typecheck dependencies) to the configuration.

I have not added the ability to add a typecheck task to .cirrus.yml when updating existing projects since that would require us parsing YAML and therefore another dependency.

It is up for discussion if we don't want to support b, c or d, but I have the feeling that we don't have much option for a (unless we opt for not having a --typed flag).

[CarliJoy]

@abravalheri thank you for all the work. Also haven't had time to check it before.
I have a idea to further reduce complexity but I am not sure if it will work:
In my former company I included the type checking by default in the CI/CD gitlab pipeline.
I only added a switch in which the pipeline won't fail. This way the users is still made aware of typing errors but it is not enforced to fix them.

In Cirrus this might work as well:
https://cirrus-ci.org/guide/writing-tasks/#failure-toleration

In Tox this is available as well:
https://tox.wiki/en/2.5.0/config.html#confval-ignore_errors=True|False(default)

So the idea would be make typing default and only have the switch change the little variable / add the typed file.
This would reduce the complexity and aim more into the typed future that @FlorianWilhelm was describing and I am sharing as well.

[abravalheri]

Thank you very much for the suggestions @CarliJoy!
Yeap, we can go for a "non-breaking"/just-informative CI action. What do you think @FlorianWilhelm.

In the future, what I was thinking is:

• Add a pyscaffold.file_modifiers module to help with the issue of adding/replacing/removing/commenting-out parts of the text in existing files.
  • This seems to be a "missing utility" for extensions (e.g. pyscaffoldext-markdown adds stuff to docs/config.py, pre-commit currently modifies the README, and the typed extension could comment/uncomment parts of the .cirrus.yml file).
• Rely on .cirrus.star and create a shared starlark library to do autodiscovery of some tasks (e.g. just run the type check if a person have a py.typed file).
  • A separated starlark lib would also simplify the amount of information a user needs to see for the Cirrus configuration, which would make it less scary to change for starters.

Of course, those things need to be debated and mainly I need some time to develop them, but I think it could be cool.

[FlorianWilhelm]

Thanks @CarliJoy, I like your solution. So typing by default, a switch for the checks in PyScaffold and in total not much more complexity. Sounds really good :-)

[abravalheri]

@FlorianWilhelm, do you prefer always performing the typecheck in Cirrus and just failing silently, or use a environment var and only_if: $TYPE_CHECKING == 'true' like it is implemented in this PR?

(Both are basically changing a switch)

[FlorianWilhelm]

In the future, what I was thinking is:

• Add a pyscaffold.file_modifiers module to help with the issue of adding/replacing/removing/commenting-out parts of the text in existing files.
  • This seems to be a "missing utility" for extensions (e.g. pyscaffoldext-markdown adds stuff to docs/config.py, pre-commit currently modifies the README, and the typed extension could comment/uncomment parts of the .cirrus.yml file).

Will pyscaffold.file_modifiers use the functionality of other libraries like pyaml, configupdater etc. to do the modifications? For Python files doing modifications consistently will be extremely hard I guess. Would use use RegExes? If an extension would use this feature, even small changes in PyScaffold's templates will most likely lead to strange bugs in the extensions. Sorry for being so pessimistic here, but I really do believe that a powerful feature like this, might lead to a lot of headaches later on.

• Rely on .cirrus.star and create a shared starlark library to do autodiscovery of some tasks (e.g. just run the type check if a person have a py.typed file).

  • A separated starlark lib would also simplify the amount of information a user needs to see for the Cirrus configuration, which would make it less scary to change for starters.

Of course, those things need to be debated and mainly I need some time to develop them, but I think it could be cool.

Sounds cool. So this would even automize then the switch @CarliJoy mentions. So .cirrus.yml would import this lib then and just use its functionality? Sounds good :-) Thanks.

[FlorianWilhelm]

@FlorianWilhelm, do you prefer always performing the typecheck in Cirrus and just failing silently, or use a environment var and only_if: $TYPE_CHECKING == 'true' like it is implemented in this PR?

(Both are basically changing a switch)

Failing silently doesn't sound so good. only_if: $TYPE_CHECKING == 'true' sounds better so that a user can switch it on explicitly and if switched on it really fails if something is wrong.

[abravalheri]

Will pyscaffold.file_modifiers use the functionality of other libraries like pyaml, configupdater etc. to do the modifications?

My idea here is to simply define the API in terms of strings. If the modifier function wants to use libraries, it should parse the input and dump the output... This is more or less what I made in this PR:

pyscaffold/src/pyscaffold/extensions/typed.py

Lines 102 to 118 in a332715

	def add_typecheck_tox(contents: FileContents, opts: ScaffoldOpts) -> FileContents:
	if not contents:
	return contents
	toxini = ConfigUpdater().read_string(contents or "")
	if "testenv:typecheck" in toxini:
	typecheck = toxini["testenv:typecheck"]
	else:
	typecheck = _section_from_template("tox_ini", "testenv:typecheck", opts)
	toxini.add_section(typecheck)
	deps_list: List[str] = opts.get("typecheck_deps", [])
	dependencies = typecheck.get("deps", Object(value=""))
	dependencies = deps.add(deps.split(dependencies.value), deps_list)
	typecheck.set("deps")
	typecheck["deps"].set_values(dependencies)
	return str(toxini)

(and what we are doing in the update module)

For Python files doing modifications consistently will be extremely hard I guess.

I was thinking more in terms of .cfg and .toml files, since there are libraries that handle it well. This is mainly motivated by the idea of migrating all the configuration to pyproject.toml. Each extension would need to add stuff in there...

But yes, I agree that changing Python code is complicated... Nevertheless we are already doing that for docs/config.py in pyscaffoldext-markdown (and if I am not wrong in pyscaffold-django). If someone wants to be on the safe site, they would need to use something like libCST or refactor

Sounds cool. So this would even automize then the switch @CarliJoy mentions. So .cirrus.yml would import this lib then and just use its functionality? Sounds good :-) Thanks.

Yes that is the idea, but it is for the future 😝, when I have sometime...
It would simplify a lot the CirrusCI configuration, by abstracting things away.

[abravalheri]

Sorry, I messed up the idea between pyscaffold.file_modifiers and the idea of having something like pyscaffold.structure.modify_contents.

For pyscaffold.file_modifiers I was planning something simple, like:

• given 2 regexes (one for start and one for the end)
  • comment everything in between
  • remove everything in between
  • replace everything in between with the new text
• given 1 regex:
  • insert text before
  • insert text after
  • remove line.

It would be more or less a extraction of what we use in pyscaffoldext-markdown, pyscaffold-django.

[FlorianWilhelm]

Thanks, looks good and sorry for the long wait :-)

[abravalheri]

Thank you very much for the review

No problems, I will leave this change marinating for a while until I have the time to proper plan for a release 😝

Maybe rethink somethings...