Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add types #507

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP] Add types #507

wants to merge 1 commit into from

Conversation

yorickvP
Copy link
Member

This PR will add types to places it is relevant.
Mostly the database layer. If we don't have schemas in the DB, this is the best we can do for now. It also eliminates direct DB access in other places.

Advantages:

  • more information for people using the database
  • typechecking finds potential problems like
kn/leden/entities.py:538: error: Item "None" of "Union[Group, User, Study, Institute, Tag, Brand, Entity, None]" has no attribute "has_tag_name"
  • We can use this for other stuff (automatic API generation, for example)

Disadvantages:

  • some extra work when you touch the db layer
  • code not fully typechecked yet, django version is too old
  • requires python 3.5-3.6. Mailman released 3.2, so we can update.

@aykevl
Copy link
Contributor

aykevl commented Aug 28, 2018

Fijn dat je naar types hebt gekregen!
Ik denk alleen niet dat dit de beste oplossing is (zie ook hier beneden). Als we types aan de database willen toevoegen, zou ik eerder de Django database abstractielaag gebruiken, oftewel SQL. Dat is een pittige hoeveelheid werk om over te zetten, maar zorgt wel voor een vast dataformaat dat ook gedocumenteerd is in de models, zie: https://docs.djangoproject.com/en/2.1/topics/db/models/

Heel eerlijk gezegd zie ik niet de toegevoegde waarde van types aan het bestaande systeem. Het lijkt me wat riskant (kans op nieuwe bugs) en types worden sowieso nauwelijks gebruikt in Python. Als we opnieuw zouden beginnen in bijvoorbeeld Go zou ik types wel erg nuttig vinden, maar ik denk eigenlijk dat het voor nu meer hindert dan het ons helpt.

requires python 3.5-3.6. Mailman released 3.2, so we can update.

Een Mailman 3 eis zou een blocker zijn, wij gebruiken nu Mailman 2 want dat zit in Debian. Pas met de buster upgrade (over ~1 jaar) kunnen we upgraden naar Mailman 3.
Overigens zit Mailman 3 in stretch-backports maar aangezien mailman een vrij kritiek stuk software is voor Karpe Noktem wil ik heel graag iets in Debian stable gebruiken, vooral voor iets dat meer "nice to have" is dan dat het echt wat toevoegt aan het bestaande systeem.

@yorickvP yorickvP marked this pull request as draft March 21, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants