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

Importer improvements #1855

Merged
merged 6 commits into from
Feb 20, 2023

Conversation

richardebeling
Copy link
Member

Fixes #1574. Best served by commit.

@richardebeling richardebeling force-pushed the import-improvements branch 2 times, most recently from 4a971b7 to e2c2884 Compare January 29, 2023 16:10
@richardebeling
Copy link
Member Author

richardebeling commented Jan 29, 2023

@niklasmohrin mypy and the invalid-value-is-a-type approach actually caught an error here that tests did not cover and we might have missed: In the checker, I first forgot to ignore rows where parsing the degree had failed (-> InvalidValue). The tests that went this path never checked whether any unexpected errors occured, so they passed because their expected error on degree parsing existed, and they simply ignored the unexpected error.

I've made the tests more assertive regarding unexpected errors in the last commit. Removing the branch in the checker now also causes a few tests to fail.

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.

Looks good, some comments:

evap/staff/importers/enrollment.py Outdated Show resolved Hide resolved
evap/staff/importers/enrollment.py Outdated Show resolved Hide resolved
evap/staff/importers/base.py Outdated Show resolved Hide resolved
evap/staff/importers/enrollment.py Outdated Show resolved Hide resolved
evap/staff/tests/test_importers.py Show resolved Hide resolved
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

✔️ Meets requirements
✔️ UI functionality checked

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.

Looks good, final remarks:

pyproject.toml Outdated Show resolved Hide resolved
evap/staff/tests/test_importers.py Show resolved Hide resolved
evap/staff/importers/enrollment.py Outdated Show resolved Hide resolved
evap/staff/importers/base.py Outdated Show resolved Hide resolved
@richardebeling richardebeling merged commit 298c9b8 into e-valuation:main Feb 20, 2023
@richardebeling richardebeling deleted the import-improvements branch February 20, 2023 16:35
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.

Import data test improvements
4 participants