Skip to content

Conversation

@valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Nov 10, 2022

See if it sorts out test issues (I'll edit this PR description if we decide to merge this)
Edit: it do

@schlunma makes a good case for this PR to be merged in #1788 (comment)

Closes #1788

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #1787 (1eea760) into main (b880462) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1787   +/-   ##
=======================================
  Coverage   91.11%   91.11%           
=======================================
  Files         203      203           
  Lines       10908    10908           
=======================================
  Hits         9939     9939           
  Misses        969      969           

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

@valeriupredoi valeriupredoi changed the title Pin mypy to not 0.10.1 Pin pytest-mypy to not 0.10.1 Nov 10, 2022
@valeriupredoi valeriupredoi marked this pull request as ready for review November 10, 2022 15:28
@valeriupredoi
Copy link
Contributor Author

OK fellas @schlunma @bouweandela dis ready for a merge 🔨

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>
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.

Looks good to me!

Please also open an issue about unpinning mypy after merging #1769 so that we don't forget this, this should only be a temporary solution.

@bouweandela please merge if you are happy with this procedure

@schlunma schlunma changed the title Pin pytest-mypy to not 0.10.1 Pin pytest-mypy to less than 0.10.1 Nov 10, 2022
@valeriupredoi
Copy link
Contributor Author

great cheers Manu - this once merged we need to merge main in #1786 which has suddenly become very important 👍

Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Let's not pin this, and instead follow the guidance at https://github.com/hauntsaninja/no_implicit_optional (which is linked from the error message) and simply set implicit_optional = True.

@zklaus
Copy link

zklaus commented Nov 11, 2022

(Of course, better yet, fix our annotations following the same repo guidance, though that is marginally more effort.)

@bouweandela
Copy link
Member

Thanks for thinking about the potential merge conflicts @schlunma! Let's follow the advice of @zklaus and then I will take care of fixing the affected type hints before the next relase.

@valeriupredoi
Copy link
Contributor Author

OK so I close this then open an implicit ninja PR? 🥷

@schlunma
Copy link
Contributor

Or change it in this PR and change the title? 😄

@valeriupredoi
Copy link
Contributor Author

following advice from @zklaus I opened #1790 and closing this shop

@valeriupredoi valeriupredoi deleted the pin_mypy branch November 11, 2022 13:09
@valeriupredoi
Copy link
Contributor Author

Or change it in this PR and change the title? smile

naah, too much fuss - plus, it's good to have a cleaner closed PR than mess about with its history 👍

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

5 participants