DH-IT-Portal-Development / ethics

Ethical Committee web application in Django
http://fetc.hum.uu.nl
MIT License
2 stars 1 forks source link

Refactor constant + tuple based choices to models.xChoices #580

Closed tymees closed 1 year ago

tymees commented 1 year ago

Django 3 added special enumeration types for choices. We're not using them, as the app is old enough to have known both Django 1.6 and Python 2 :P

However, for models with multiple sets of choices it's somewhat hard to figure out which ones belong together. Using these enumeration types will make things more clear.

The refactor can be done without migrations, if done correctly. (As those enumeration types generate the same format as the old method in the end).

Not a priority, just a nice to have.

EdoStorm96 commented 1 year ago

I'd happily take this up. I see why it's nice to have, but it will be a bit of work, since, using review.stage as an example, assuming the creation of a subclass called Stages:

Querysets would change from Review.objects.filter(stage=Review.SUPERVISOR) to Review.objects.filter(stage=Review.Stages.SUPERVISOR). Other comparisons would of course change in the same way. So this would also require quite some changes outside of the models.

Would you suggest only updating the choices for models with multiple sets of choices? Or doing so for the whole repo? for instance YES_NO_DOUBT can also be changed to a models.TextChoices class.

In my initial test, with changing the choices for review.stage to the new format, it indeed does not require migrations, when performing a dry run.

tymees commented 1 year ago

Ideally we'd convert everything; no sense using two different methods.

It's a lot of work, yes, but hunting down all references should not be that hard. Dunno how well VSCode handles it, but my IDE can give me a list of all usages of, for example, Review.SUPERVISOR. And with some regexp con-fu, refactoring can also be largely automated. However, that last one requires some skill I guess.

If your IDE cannot figure out all usages, this might be a good exercise in using grep (or ack). grep -R 'Review\.[A-Z\_]*' . should give you all usages of constants of Review. It's a very nice CLI tool to understand, I use it daily.

However, doing this in stages is also fine imo; it's not a priority in any sense