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

API-Anpassungen: Wrapper-Objekte #111

Merged
merged 14 commits into from
Sep 27, 2024
Merged

API-Anpassungen: Wrapper-Objekte #111

merged 14 commits into from
Sep 27, 2024

Conversation

MHajoha
Copy link
Member

@MHajoha MHajoha commented Jun 24, 2024

Bisher noch out of scope:

  • Question.prefix
  • create_placeholder
  • create_input u.ä.
  • call_js & add_css
  • Hints

Etwas anders als in questionpy:api befassen sich die High-Level Klassen (Question und Attempt) nur mit geparsten States. Wollen Paketentwickler:innen die String-Repräsentation ihrer States beeinflussen, können relativ einfach die Wrapper gesubclassed oder in die Pydantic-Werkzeugkiste gegriffen werden (z.B. mit eigenen Validators bzw. Serializern auf der State-Klasse).

Copy link
Contributor

@MartinGauk MartinGauk left a comment

Choose a reason for hiding this comment

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

Bezüglich nur geparste/validierte States an Question/Attempt bin ich mir unsicher. Wir müssen an den Fall denken, dass eine Frage in eine andere Frage eingebettet wird. Dazu muss sämtliche Logik zum Validieren und Dumpen in den Klassen selber liegen (bzw. idealerweise die Pydantic-Werkzeugkiste genutzt werden).

Meine Vorstellung war, dass das JSON-Dumpen/Parsen (von/nach JsonValue) im Wrapper geschieht. Zwischen der Frage und dessen Subquestions kann dann der Austausch der States in Form von JsonValues geschehen. Ich denke hier an den Fall, dass die Frage auf einfache Weise die States der Subquestions in den eigenen State aufnehmen können muss.

@MHajoha
Copy link
Member Author

MHajoha commented Jul 11, 2024

Attempt und Question übernehmen jetzt auch das Validaten vom JSON zum Objekt. Ich Frage mich gerade noch, ob es eine Methode Attempt.from_plain_states(question, attempt_state: dict[...], scoring_state: dict[...] | None, response: dict[...] | None) geben sollte. Das wäre zwar mit Blick auf Question.from_plain_state konsistent, würde aber das gleiche machen wie Question.get_attempt (die dann vermutlich einfach nur delegieren würde). Habt ihr dazu Meinungen?

@MHajoha MHajoha requested a review from tumidi August 1, 2024 10:27
Copy link
Contributor

@MartinGauk MartinGauk left a comment

Choose a reason for hiding this comment

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

Du weichst von der ursprünglichen Planung in Tefen ab, in der die Methoden Question.start_attempt -> AttemptStartedModel, get_attempt -> AttemptModel und score_attempt -> AttemptScoredModel vorgesehen waren. Warum?

Ich bin mir selber nicht mehr sicher, warum ich das so hatte. Ein Vorteil wäre, dass wir dann den Paketen das Aussehen der Attempt-Klasse nicht vorschreiben würden.

Ich glaube meine Verunsicherung kommt auch daher, dass ein externer Akteur zuerst formulation und danach erst score_response aufrufen könnte.

Bezüglich deiner Frage zu Attempt.from_plain_states: Ich denke wie es jetzt ist, ist es schon in Ordnung, solange wir nicht doch noch Question.score_attempt einführen sollten.

Examples:
This example shows how to subclass [`QuestionType`][questionpy.QuestionType]:
def start_attempt(self, variant: int) -> Attempt:
attempt_state = self.attempt_class.make_attempt_state(self, variant)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ich finde es zwar logisch, die Logik zum Erstellen eines neuen attempt_state in der Attempt-Klasse (als classmethod) unterzubringen, aber in Verbindung mit typing hat das den Nachteil, dass man in einer eigenen make_attempt_state Implementierung seine eigene Question-Klasse nicht angeben kann, ohne dass mypy meckern wird.
Die Wahrscheinlichkeit ist ja sehr hoch, dass man mind. auf die Options zugreifen will, um den attempt_state zu erzeugen.

(Das ist kein Blocker, der das Mergen verhindert. Darum können wir uns auch an anderer Stelle kümmern.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Das Problem trifft tatsächlich auch auf Question.make_question_state zu. make_attempt_state war ein Versuch, damit Symmetrie herzustellen. Eine Lösung habe ich allerdings nicht parat. Wenn mypy PEP 695 unterstützt, könnten wir nochmal die Verwendung von Generics eroieren.

@MHajoha
Copy link
Member Author

MHajoha commented Aug 13, 2024

Du weichst von der ursprünglichen Planung in Tefen ab, in der die Methoden Question.start_attempt -> AttemptStartedModel, get_attempt -> AttemptModel und score_attempt -> AttemptScoredModel vorgesehen waren. Warum?

Ich glaube mein Ziel war es, dass die Models weitgehend nur auf QuestionInterface-Ebene und darüber genutzt werden, und die Question-Ebene konsistent nur mit Properties arbeitet. Das klarifiziert denke ich die Trennung der beiden Ebenen.

Ich bin mir selber nicht mehr sicher, warum ich das so hatte. Ein Vorteil wäre, dass wir dann den Paketen das Aussehen der Attempt-Klasse nicht vorschreiben würden.

Aktuell schreiben wir ja schon große Teile der Attempt-Klasse in Attempt vor. Zwar wäre es möglich, auf Duck-Typing zu setzen und eine stark Abweichende Attempt-Klasse zu bauen, die keine Subklasse von Attempt ist, das habe ich bisher aber nicht als Feature angesehen.

@MHajoha MHajoha requested a review from MartinGauk August 13, 2024 13:54
@MartinGauk
Copy link
Contributor

Ja, so richtig schön ist das nicht, wenn wir hier Models verwenden würden. Ich finde die Methoden Question.start_attempt/get_attempt/score_attempt aber trotzdem recht sinnvoll. Wir könnten hier ja auch Protocols verwenden und alle public read-only properties und die to_plain_attempt_state/to_plain_scoring_state Methoden darin deklarieren.
In Wahrheit würde man zwar immer ein Objekt derselben Attempt-Klasse zurückgeben, aber nach außen hin wäre klar, dass man u.a. score nur bei score_attempt erhält.

Mir ist das Design an der Stelle recht wichtig, weil man die Attempts ggf. auch als Unterfrage in einer eigenen Frage verwenden will.

@MHajoha
Copy link
Member Author

MHajoha commented Sep 23, 2024

Ja, so richtig schön ist das nicht, wenn wir hier Models verwenden würden. Ich finde die Methoden Question.start_attempt/get_attempt/score_attempt aber trotzdem recht sinnvoll. Wir könnten hier ja auch Protocols verwenden und alle public read-only properties und die to_plain_attempt_state/to_plain_scoring_state Methoden darin deklarieren. In Wahrheit würde man zwar immer ein Objekt derselben Attempt-Klasse zurückgeben, aber nach außen hin wäre klar, dass man u.a. score nur bei score_attempt erhält.

Habe ich in 89deaa0 mal testweise umgesetzt. Wenn Attempt die Protocols explizit als Superklassen deklariert, schlägt Mypy an, weil es score: float | None als "settable attribute" und @property ... als "read-only property" interpretiert und das eine nicht mit dem anderen overriden/implementieren lässt. Daher ohne.

Copy link
Contributor

@MartinGauk MartinGauk left a comment

Choose a reason for hiding this comment

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

Danke dir

@MHajoha MHajoha merged commit d3a0489 into dev Sep 27, 2024
6 checks passed
@MHajoha MHajoha deleted the api-changes branch September 27, 2024 13:45
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.

None yet

3 participants