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

upload updated mypy.ini if needed #1624

Merged
merged 2 commits into from
Mar 23, 2021
Merged

upload updated mypy.ini if needed #1624

merged 2 commits into from
Mar 23, 2021

Conversation

ahartikainen
Copy link
Contributor

Description

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #1624 (0d52e39) into main (9420925) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1624   +/-   ##
=======================================
  Coverage   90.92%   90.92%           
=======================================
  Files         108      108           
  Lines       11663    11663           
=======================================
  Hits        10604    10604           
  Misses       1059     1059           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9420925...0d52e39. Read the comment docs.

@ahartikainen
Copy link
Contributor Author

Hey, it works. If someone else could confirm this too I can remove those non-needed commits and we can merge this.

@canyon289
Copy link
Member

So I read this but I dont really get what it does and am confused that you say it works but CI is failing?

For specifica feedback, can we add a comment to yml file explaining what this thing is, even one line helps
And in regards to CI if its failing how do you know it works?

@OriolAbril
Copy link
Member

OriolAbril commented Mar 20, 2021

We recently added mypy and typing copilot to CI. mypy checks that the code does not incur in any type errors, which sounds great, however, many parts of our codebase and many of our dependencies do not use type hints everywhere (or at all). Therefore, mypy check relies on a very long and specific mypy.ini file that ensures we make the most of the type hints available without getting any errors due to the missing type hints. typing copilot is what takes care of this file. typing copilot was first used to generate a base mypy.ini file, and now it is also executed on CI to check if we can "tighten" this file, that is, remove some of the ignores because we our our dependencies added type hints. If we can tighten, typing copilot will fail the CI job! This is what is happening now, ci is failing because we can improve our mypy.ini.

The improvement Ari is adding with this PR is that now, whenever typing copilot fails ci, azure will rerun typing copilot, but not as a check, as mypy.ini generator and upload the improved mypy.ini as an artifact. Thus, "fixing" the failing CI will become downloading the updated mypy.ini from azure and adding it to the repo.

Regarding explanation, I'd rather have something added to the dev guide instead of comments, we can all update #1587

@OriolAbril
Copy link
Member

I can confirm that the mypy.ini is uploaded and looks great, thanks for this @ahartikainen

@ahartikainen
Copy link
Contributor Author

Do we add a mention about updating mypy.ini in #1587 or here?

@OriolAbril
Copy link
Member

I would do it at #1587, still trying to decide how to organize the doc, but I think we should probably have a section about CI

@obi1kenobi
Copy link
Contributor

Very cool!

@OriolAbril OriolAbril merged commit 7ec650f into main Mar 23, 2021
@OriolAbril OriolAbril deleted the ci/upload_mypy_ini branch March 23, 2021 17:06
utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
* upload updated mypy.ini if needed

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants