-
Notifications
You must be signed in to change notification settings - Fork 147
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
type results exporter #2035
type results exporter #2035
Conversation
typechecker fails => this is fixed in #2064 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks :)
This seems to be running into the same stuff that is solved in #2091 now, I would say we wait until that is done? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to also review your code yourself before requesting the a review, you yourself are most familiar with the code and can sort out low hanging fruit most quickly
Apart from the open discussions, this looks good to me :) |
I unresolved some comments that still seem unanswered to me, could you give a quick response to them? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there :) Please remember to perform self-reviews, it's usually worth it (that is also my experience with code I write myself)
f266cf7
to
6619c31
Compare
6619c31
to
aae8328
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfection!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. There's still some minor issues left (T
should probably be ModelT
, degrees
vs degree_ids
, course_types
vs course_types_ids
, a -> None
is missing, I think one : list[bool]
annotation is superfluous), but I think we can already merge this.
this PR makes work for #2001 easier