maurerle / eralchemy2

Entity Relation Diagrams generation tool based on https://github.com/Alexis-benoist/eralchemy
Apache License 2.0
63 stars 15 forks source link

Finalize typing #12

Closed kasium closed 1 year ago

kasium commented 1 year ago

Closes #7

kasium commented 1 year ago

@maurerle can you please support? Should I put black, isort, mypy into tox? If yes, can you please give a hint? Else I need to install poetry and the dev dependencies in the CI. For that I know how to do it 😄

maurerle commented 1 year ago

Well good question. Currently I added the extras dependencies to the extra ci Tox does not install the dev-dependencies when installing the package

We could either

  1. add poetry install to commands, resulting in added dev-dependencies, and list the CI packages there
    • this does not catch up the additional tox deps like SQLAlchemy<1.4
  2. add black, isort, mypy to the tox deps, and add them to the dev-dependencies, resulting in two declarations of the same package
  3. add black, isort, mypy to the ci extra packages, but then they must be added to normal dependencies with optional: true too (resulting in two declarations of the package, or even three as they are also in dev-dependencies)

I am quite new to poetry, so maybe there is an even better way I did not see. But I think, that the third option is the best one.

It's a bit of a pity that tox does not have an easy way to install the poetry dev dependencies.

kasium commented 1 year ago

I had the same issue in https://github.com/SAP/swagger-plugin-for-sphinx and went to install poetry with dev dependencies in the CI

maurerle commented 1 year ago

I see, but this does not really work with tox, as it manages a virtualenv itself and the testing dependency would be overwritten: https://github.com/maurerle/eralchemy2/blob/main/pyproject.toml#L74

Furthermore if poetry install happens only in ci you have to do that in tox too prior to running tox, and I remember that something was weird that way yesterday..

So for me I think that 3 is the cleanest way in my case?

kasium commented 1 year ago

Mhm, sounds like a lot of duplication to me. One option would be to remove tox and just to test directly? What di you think about that? Also maybe https://github.com/tkukushkin/tox-poetry could help.

In the end please do what you think is the best for the project

maurerle commented 1 year ago

The problem I have is, that I'd like to test it once with SQLAlchemy < 1.4 and once above with the current version. Somehow tox-poetry also seemed to just install all the dev-dependencies resulting in SQLAlchemy >=1.4

So if someone has a better way I am all ears but for now i just installed black and isort outside of tox and run it afterwards.

Thank you @kasium for the types!

kasium commented 1 year ago

@maurerle one approach would be to get rid of tox and let GitHub Actions install the correct python version

maurerle commented 1 year ago

the problem is not the different python versions but the two environments for SQLAlchemy: https://github.com/maurerle/eralchemy2/blob/main/pyproject.toml#L74

Actually I am already using github actions to take care of the python version: https://github.com/maurerle/eralchemy2/blob/main/.github/workflows/python-app.yml#L20

How would you suggest to add the two test cases with SQLAlchemy < 1.4 and >=1.4 without tox?

kasium commented 1 year ago

Beside using more flexible solutions like nox you can move that to the actions itself. So the action itself installs Sqlalchemy with the desired version and then executes the tests