Skip to content

Conversation

@valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Nov 11, 2022

Description

Suggested by @zklaus in #1787 (review) which this PR supersedes #1787

Closes #1788


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@valeriupredoi
Copy link
Contributor Author

OK it looks like that did the trick partially: from 9 to 1 failed mypy test, can I get some help from the standards experts @zklaus @bouweandela @schlunma please - am no good at these things, and wouldn't know what to fix now 🍺

@schlunma
Copy link
Contributor

You could either try using the option --check-untyped-defs as suggested or remove the type hints here:

running: Dict[Type[BaseTask], Type[ApplyResult]] = {}

and add them to a comment maybe?

@valeriupredoi
Copy link
Contributor Author

well that escalated real quick - I'll see about removing the Type from _task.py 🤦‍♂️

@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #1790 (5442a76) into main (b880462) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1790      +/-   ##
==========================================
- Coverage   91.11%   91.11%   -0.01%     
==========================================
  Files         203      203              
  Lines       10908    10906       -2     
==========================================
- Hits         9939     9937       -2     
  Misses        969      969              
Impacted Files Coverage Δ
esmvalcore/_task.py 72.23% <100.00%> (-0.13%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@valeriupredoi
Copy link
Contributor Author

OK so I eventually got this through the mypy hoops but am not sure of the functionality I hacked out - @bouweandela could you have a look please? 🍺

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Thanks V.! 🚀

Sorry, this change is exactly what I meant by "remove the type hints", could have been clearer!

@valeriupredoi
Copy link
Contributor Author

cheers, Manu! Can I nominate @bouweandela to have a looksee at it and merge, please 🍺

@bouweandela bouweandela merged commit 4527173 into main Nov 11, 2022
@bouweandela bouweandela deleted the set_implicit_optional_mypy branch November 11, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tests failing left, right, and centre on Circle and Github Actions

4 participants