IBM / python-itoolkit

itoolkit is a Python interface to the XMLSERVICE toolkit for the IBM i platform.
MIT License
19 stars 14 forks source link

Add type declarations #59

Open kadler opened 4 years ago

kadler commented 4 years ago

https://mypy.readthedocs.io/en/latest/cheat_sheet_py3.html https://mypy.readthedocs.io/en/latest/existing_code.html

This would allow us to have better docs using https://pypi.org/project/sphinx-autodoc-typehints/

Ideally, we would use inline type declarations, but they can't be used if the code needs to be Python 2 compatible. For Python 2 compatibility, you must use comments, but this requires Python 3.8 or typed-ast be installed in order for sphinx-autodoc-typehints to be able to read them.

jkyeung commented 4 years ago

If you or active volunteers (I don't count myself as active) really want this, I won't object, but I won't be a fan.

I am in the camp that has always been and will always remain against in-line type annotations in Python. I think they really destroy the readability of the language, and we don't even get type enforcement or static-typing speed out of the deal. (Unlike many controversial PEPs, PEP 484 doesn't even mention the opposition, but it was vociferous on the python-dev list.)

I would be fully supportive of comment-based (or stub file) type annotations. I think the requirement for Python 3.8 or typed-ast isn't particularly onerous, especially given that it would only be imposed on people who are already willing, able, and invested in setting up and using Sphinx.

kadler commented 4 years ago

Thanks for the input, @jkyeung. I'm very new to type hints, so I'm not sure the approach that I'd want to take just yet.

The biggest concern I had with Python 3.8 / typed-ast was whether that was supported by readthedocs.io, since that's where the documentation is hosted. I agree that setting up a Python 3.8 environment to build docs is not all that onerous, especially since very few people would be doing so.

jwoehr commented 1 year ago

Python 2 is no longer an issue, correct? Expired budgie.

kadler commented 1 year ago

Yeah, the current development release (2.0.0-dev) is Python 3.6+

kadler commented 1 year ago

The v1 branch still "supports" Python 2, but I don't anticipate many changes there.

jwoehr commented 1 year ago

I support the plan to add type annotation. I don't currently have time to do that work, but I can help review it. Or possibly later this year I may have time to work on it.

jwoehr commented 1 year ago

@kadler is anyone working on this? @alanseiden and I were chatting on this today and I forked and converted a file. Should I proceed thru the tree?

jwoehr commented 1 year ago

Trying to run tests but get error:

 $ python -m pytest tests
ERROR: usage: __main__.py [options] [file_or_dir] [file_or_dir] [...]
__main__.py: error: unrecognized arguments: --cov=itoolkit
  inifile: /home/jwoehr/work/python-itoolkit/setup.cfg
  rootdir: /home/jwoehr/work/python-itoolkit

Any tips?

kadler commented 1 year ago

IIRC, you need pytest-cov installed. Been a while, but IIRC you can use something like poetry run python -m pytest tests since Poetry manages the venvs for you.

kadler commented 1 year ago

@jwoehr feel free to work on it if you like!

jwoehr commented 1 year ago

Thanks, that helps. Doing better now, 87 passed, but 16 fail with an error of the form:

_________________ ERROR at setup of test_idb2call_with_ibm_db __________________
file /home/jwoehr/work/python-itoolkit/tests/test_unit_idb2call.py, line 41
  def test_idb2call_with_ibm_db(mocker, database_callproc):
E       fixture 'mocker' not found
>       available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, cov, database, database_callproc, database_close_exception, database_execute, doctest_namespace, monkeypatch, no_cover, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory, transport
>       use 'pytest --fixtures [testpath]' for help on them.

/home/jwoehr/work/python-itoolkit/tests/test_unit_idb2call.py:41

mocker is installed

kadler commented 1 year ago

Is it installed in poetry's venv? (eg. poetry run pip env)

jwoehr commented 1 year ago
(test_venv) jwoehr@pop-os:~/work/python-itoolkit$ poetry run pip env
ERROR: unknown command "env"
(test_venv) jwoehr@pop-os:~/work/python-itoolkit$ poetry run pip list --local
Package        Version
-------------- ----------
coverage       7.2.7
exceptiongroup 1.1.1
iniconfig      2.0.0
itoolkit       2.0.0.dev0
mock           5.0.2
packaging      23.1
pip            23.1.2
pluggy         1.2.0
pytest         7.4.0
pytest-cov     4.1.0
PyYAML         6.0
setuptools     59.6.0
tomli          2.0.1
wheel          0.40.0
kadler commented 1 year ago

Sorry, I gave the wrong command but you figured out what I meant.

Hmm, looks at the error message closer it looks like test_idb2call_with_ibm_db is taking a fixture called "mocker". Seems to be provided by pytest-mock, which is not installed in your venv. It's listed in the dev-dependencies in pyproject.toml, though. Seems like they changed things since Poetry 1.2, which uses dependency groups instead now but should still be backward compatible.

Maybe try poetry install --with dev?

kadler commented 1 year ago

I got it set up on my new system. Here's what I did:

kadler commented 1 year ago

There might be a way to do this with pure pip nowadays since some recent PEPs, but I'm not sure on that process.

jwoehr commented 1 year ago

Phew. Got that to work. Side note: one must deactivate the venv one already created for poetry install to work.

--------------------------------------------------------
TOTAL                                  700    247    65%

======================= 103 passed, 2 warnings in 0.33s ========================