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

idm med tktitler #127

Merged
merged 26 commits into from
Jan 24, 2017
Merged

idm med tktitler #127

merged 26 commits into from
Jan 24, 2017

Conversation

neic
Copy link
Member

@neic neic commented Jan 7, 2017

Et første bud på hvordan idm kan bruge tktitler.

Jeg vil mene at de fleste funktioner under Title-classen skal fjernes. Ideen var at flytte valget af præsentationen af en titel længere op. Måske er Title.titletupel og Title.parse nok. Jeg er dog ikke sikker på hvordan regnskab afhænger af det endnu.

@neic neic requested a review from Mortal January 7, 2017 13:27
@neic
Copy link
Member Author

neic commented Jan 7, 2017

Navnet Title.titletupel er også en del af debatten.

Mortal
Mortal previously requested changes Jan 9, 2017
@@ -236,6 +96,9 @@ class Title(models.Model):
root = models.CharField(max_length=10, verbose_name='Titel')
kind = models.CharField(max_length=10, choices=KIND, verbose_name='Slags')

def titletupel(self):
return (self.root, self.period)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Tupel" er ikke engelsk. Jeg synes det bør hedde "title_tuple".

@@ -14,3 +14,4 @@ django-macros>=0.4.0
unidecode>=0.04.19
mysqlclient>=1.3.7
django-multiupload==0.5.2
git+git://github.com/TK-IT/tktitler.git@v0.2.1#egg=tktitler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Da prodekanus er firewalled bør vi bruge git+https i requirements.

@neic
Copy link
Member Author

neic commented Jan 12, 2017

Måske er jeg on a limb: Hvis vi gør Title iterable kan vi give et Title-objekt direkte til tktitler-funktionerne uden at kalde .title_tuple() på det.

class Title(models.Model):
    #[…]

    def __iter__(self):
        return iter((self.root, self.period))

someTitle = Title.objects.all()[0]
tktitler.postfix(someTitle)  # "FUØP16"

Jeg ved ikke om der er nogle problemer i at gøre en Django model iterabel. Eller om det overhovedet giver mening.

@Mortal
Copy link
Contributor

Mortal commented Jan 12, 2017

Jeg synes det er dårligt at gøre Title iterabel, fordi Explicit is better than implicit.

Jeg vil hellere have at tktitler tjekker for en titel_tuple metode på inputtet - og eventuelt kan tktitler erklære en abstrakt baseklasse som et objekt skal abstrakt nedarve fra før den tjekker om objektet har en titel_tuple(). Se collections.ABCMeta.

@neic
Copy link
Member Author

neic commented Jan 12, 2017

Hvis jeg forstår ABC og metaklasser korrekt, så kunne en løsning være:

# tktitler.py
#[…]
class Title(metaclass=ABCMeta):
    @abstractmethod
    def title_tuple(self):
        pass

# idm/models.py
#[…]
class Title(models.Model, tk.Title):
    #[…]
    def title_tuple(self):
        return (self.root, self.period)

Det virker dog ikke fordi Django selv bruger metaklasser og det er ikke entydigt hvilken metaklasse der skal bruges:

class Title(models.Model, tk.Title):
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

En 'løsning' er måske:

class MyMeta(ABCMeta, models.base.ModelBase):
    pass

class MyTitle(metaclass = MyMeta):
    pass

class Title(MyTitle, models.Model, tk.Title):

men jeg er ude på dybt vand. Desuden bliver det for kompleks til at det giver mening.

Er der noget jeg har overset?

@Mortal
Copy link
Contributor

Mortal commented Jan 12, 2017

Beklager jeg ikke linkede præcist nok. Jeg mener at vi kan bruge ABCMeta.register. https://docs.python.org/3.6/library/abc.html#abc.ABCMeta.register

Så behøver vi ikke ændre på hvilke klasser Title rent faktisk nedarver fra.

@neic
Copy link
Member Author

neic commented Jan 12, 2017

Om virtual subclasses:

[…] these and their descendants will be considered subclasses of the registering ABC by the built-in issubclass() function, but the registering ABC won’t show up in their MRO (Method Resolution Order) nor will method implementations defined by the registering ABC be callable (not even via super()).

Med tk.Title.register(Title) retunerer isinstance(Title.objects.all()[0], tk.Title) godt nok True, men den giver ingen fejl på instantiation hvis title_tuple() ikke er overskrevet i idm.models.Title. (Som den ville have gjort hvis det ikke var virtuel, men alm. subklasse.)

Den eneste fordel er vel så at man kan bruge isinstance(title_parameter, Title) istedet for hasattr(title_parameter, 'title_tuple') eller try…catch i prefix() et al.?

@Mortal
Copy link
Contributor

Mortal commented Jan 12, 2017

Ja, det er kun issubclass der ændres når man registrerer en ny subklasse.

Forskellen på hasattr og issubclass er at issubclass (EDIT: hasattr) ikke har nogen forståelse for navnerum, hvorimod register/issubclass har. Derfor er register bedre.

Hvis et andet interface i verden bruger metodenavnet titel_tuple, og vi blot tjekker efter metodenavnet, så kan det være at vi accepterer objekter der ikke burde accepteres. Med register skriver forfatteren bag klassen eksplicit, at de understøtter vores interface.

Det er mere kompliceret at bruge register og issubclass end at bruge hasattr, så hasattr er bedre i dén anseende. Hvad synes du er bedst?

@neic
Copy link
Member Author

neic commented Jan 12, 2017

ABCMeta.register og issubclass() er fint. Man behøver ikke at forstå metaklasser for at anvende det. Det havde bare været rart hvis vi også kunne få checket på instantiation med. Jeg laver det imorgen.

@Mortal
Copy link
Contributor

Mortal commented Jan 12, 2017

Jeg er tilfreds sålænge vi giver en meningsfyldt fejlmelding når argumentet ikke implementerer klassen. Hvis argumentet implementerer det, men ikke har den rigtige metode, så er det (efter min mening) i orden at give en fejlmelding der ikke giver mening.

@Mortal
Copy link
Contributor

Mortal commented Jan 13, 2017

Jeg har implementeret en tk.title_class decorator i tktitler/#20.

@neic neic mentioned this pull request Jan 13, 2017
11 tasks
@Mortal
Copy link
Contributor

Mortal commented Jan 22, 2017

Hvad med at sætte tktitler gfyear i en middleware? Så kan vi gå fra et sted mellem 0 og 100 kald til config.GFYEAR pr. view til (næsten) altid ét kald pr. view, og vi kan slippe for at henvise til constance de fleste steder.

Ulemperne er 1) vi tilgår databasen selv på views der ikke bruger config.GFYEAR til noget, og 2) det fremgår ikke af koden at året hentes via constance.

@neic neic merged commit 2c6c246 into master Jan 24, 2017
@neic neic deleted the tktitler branch March 9, 2018 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants