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

type results exporter #2035

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

fidoriel
Copy link
Collaborator

this PR makes work for #2001 easier

evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
@fidoriel
Copy link
Collaborator Author

typechecker fails => this is fixed in #2064

evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
richardebeling
richardebeling previously approved these changes Dec 18, 2023
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

thanks :)

evap/evaluation/tools.py Outdated Show resolved Hide resolved
@niklasmohrin
Copy link
Member

This seems to be running into the same stuff that is solved in #2091 now, I would say we wait until that is done?

@Kakadus Kakadus mentioned this pull request Dec 29, 2023
@fidoriel fidoriel requested a review from niklasmohrin January 22, 2024 17:55
Copy link
Member

@niklasmohrin niklasmohrin left a 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

evap/results/exporters.py Outdated Show resolved Hide resolved
evap/evaluation/tools.py Outdated Show resolved Hide resolved
evap/results/tools.py Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
@niklasmohrin
Copy link
Member

Apart from the open discussions, this looks good to me :)

evap/results/exporters.py Outdated Show resolved Hide resolved
@niklasmohrin
Copy link
Member

I unresolved some comments that still seem unanswered to me, could you give a quick response to them?

@fidoriel fidoriel requested a review from richardebeling June 17, 2024 17:57
Copy link
Member

@niklasmohrin niklasmohrin left a 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)

evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
evap/results/exporters.py Outdated Show resolved Hide resolved
@fidoriel fidoriel force-pushed the type-results-exporter branch 3 times, most recently from f266cf7 to 6619c31 Compare July 29, 2024 19:07
@fidoriel fidoriel requested a review from niklasmohrin July 29, 2024 19:08
@fidoriel fidoriel force-pushed the type-results-exporter branch from 6619c31 to aae8328 Compare July 29, 2024 19:10
Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Perfection!

Copy link
Member

@richardebeling richardebeling left a 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.

@richardebeling richardebeling merged commit 20e6f4a into e-valuation:main Jul 30, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants