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.

Sign up for GitHub

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

Jump to bottom

Partially added Python type hints and enabled mypy in CI #521

Merged
merged 10 commits into from
Dec 8, 2020
Merged

Partially added Python type hints and enabled mypy in CI #521

merged 10 commits into from
Dec 8, 2020

Conversation

rubenthoms
Copy link
Collaborator

This closes #483. Probably requires a new issue addressing type hinting of the remaining files.

rubenthoms and others added 6 commits December 4, 2020 11:10
@rubenthoms @sigurdp
Added mypy to CI.
06c5d36 
Excluded all plugins that are not yet type-hinted from mypy execution.

Co-authored-by: Sigurd Pettersen <sigurd.pettersen@ceetronsolutions.com>
@rubenthoms
Removed TODO tags from pylint warnings list.
2edfd8e
@rubenthoms
Removed condition in workflow file in order to have code-checks/tests…
48d6b42 
… always running.
@rubenthoms
Set fail-fast to false in workflow file.
720a47d
@rubenthoms @sigurdp
Commented out building of documentation in workflow file.
fbbee20 
Conflict with webviz-config regarding type hints.

Co-authored-by: Sigurd Pettersen <sigurd.pettersen@ceetronsolutions.com>
@rubenthoms @sigurdp
Added type hints and refactored code where necessary.
6ddb370 
- Added type hints
- Moved hex_to_rgba to _utils/colors
- Pylint fixes
- Pylint work-around for mypy related imports yielding cyclic-import errors

Co-authored-by: Sigurd Pettersen <sigurd.pettersen@ceetronsolutions.com>
anders-kiaer
anders-kiaer reviewed Dec 4, 2020
View reviewed changes
Copy link
Collaborator

@anders-kiaer anders-kiaer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Awesome work 🎉 Could not find any big things to comment on (minor comments only).

Line 18 in the GitHub workflow can perhaps be reactivated now (since it is a PR)? And we can generalize the workflow in a separate PR to also work without a PR.

@asnyv 's PR will fix webviz-config, we do a release, and can then enable 93+94 as well.

🚀

.pylintrc Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/integration_tests/test_aggregated_data_example.py Outdated Show resolved Hide resolved
@anders-kiaer anders-kiaer changed the title 483 - Partially added Python type hints and enabled mypy in CI Partially added Python type hints and enabled mypy in CI Dec 4, 2020
@rubenthoms
Adjusted order of import statements.
a2f2a83 
Added newline to .pylintrc.
HansKallekleiv
HansKallekleiv reviewed Dec 7, 2020
View reviewed changes
Copy link
Collaborator

@HansKallekleiv HansKallekleiv left a comment

Choose a reason for hiding this comment

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

Great work! LGTM

@anders-kiaer
Copy link
Collaborator

anders-kiaer commented Dec 7, 2020 •
edited
Loading

@rubenthoms @sigurdp Is CI (or local testing on your side) happy with this PR if we revert back to running webviz docs and temp. install webviz-config from master? If so - we will follow up by releasing a bug fix release of webviz-config and we take this one in 🎉

@sigurdp
Copy link
Collaborator

sigurdp commented Dec 8, 2020

Just tested running webviz docs locally with webviz-config from master installed and it is running fine here.

@rubenthoms
Copy link
Collaborator Author

Just tested running webviz docs locally with webviz-config from master installed and it is running fine here.

Same here. Ready to be taken in :) I'd wait until this is merged in before starting with the new PVT plot features in order to have the type hints included.

anders-kiaer
anders-kiaer reviewed Dec 8, 2020
View reviewed changes
Copy link
Collaborator

@anders-kiaer anders-kiaer left a comment

Choose a reason for hiding this comment

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

🎉

.github/workflows/subsurface.yml Outdated Show resolved Hide resolved
.github/workflows/subsurface.yml Outdated Show resolved Hide resolved
@rubenthoms rubenthoms force-pushed the EQ_483-Add-Python-type-hints-and-mypy branch from 3d90d0a to ed44fa4 Compare December 8, 2020 14:27
@anders-kiaer anders-kiaer marked this pull request as ready for review December 8, 2020 14:28
@anders-kiaer anders-kiaer self-requested a review December 8, 2020 14:28
anders-kiaer
anders-kiaer approved these changes Dec 8, 2020
View reviewed changes
Copy link
Collaborator

@anders-kiaer anders-kiaer left a comment

Choose a reason for hiding this comment

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

Great work @rubenthoms and @sigurdp 🎉 Feel free to merge when 🟢

@rubenthoms rubenthoms merged commit f979a53 into equinor:master Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HansKallekleiv HansKallekleiv HansKallekleiv left review comments

@anders-kiaer anders-kiaer anders-kiaer approved these changes

@asnyv asnyv Awaiting requested review from asnyv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Python type hints + enable mypy in CI
4 participants
@rubenthoms @anders-kiaer @sigurdp @HansKallekleiv