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

ENH: introduce typing checking and GitHub workflow #6885

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

yarikoptic
Copy link
Member

Initiates work toward addressing #6884

Some simply do not have any type annotations, but some do have
annotations and imports from typing, thus were selected for a "starter"
@yarikoptic yarikoptic added the semver-internal Changes only affect the internal API label Jul 24, 2022
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

I cannot speak to the changes, but I think it would be good to add comments on this type of code too. Sooner are later these checks will start to fail, and at that point everyone is expected to know what is going on, what the purpose of a check is, and also where its intended scope is. From the action specification it is not at all obvious that this type-checking is limited to very few code pieces. This check passing, means very little about the quality and comprehensiveness of typing in datalad in general, but that message won't be that with the green checkmark in a PR.

@@ -0,0 +1,27 @@
name: Type-check
Copy link
Member Author

Choose a reason for hiding this comment

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

In response to @mih #6885 (review) . Do you mean something like

Suggested change
name: Type-check
# This workflow is running types checks using `tox -e typing` you could do locally.
# As type hinting is not yet fully and consistently done through out the project only
# some selected files (listed in tox.ini) will be checked. Feel welcome to add extra
# files to the list there. See https://github.com/datalad/datalad/issues/6884 for the
# overall goal/progress towards type hints coverage in DataLad.
name: Type-check

or what?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, we improved this wording a bit to signal that selection of initial files pretty much random. The point is to extend it.

Since this is better than nothing let's proceed

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #6885 (52f1d83) into maint (b0500a5) will increase coverage by 0.95%.
The diff coverage is n/a.

❗ Current head 52f1d83 differs from pull request most recent head 48aeda5. Consider uploading reports for the commit 48aeda5 to get more accurate results

@@            Coverage Diff             @@
##            maint    #6885      +/-   ##
==========================================
+ Coverage   90.25%   91.21%   +0.95%     
==========================================
  Files         354      354              
  Lines       46116    46116              
==========================================
+ Hits        41622    42064     +442     
+ Misses       4494     4052     -442     
Impacted Files Coverage Δ
datalad/tests/utils.py 66.76% <0.00%> (+9.73%) ⬆️
datalad/tests/test_tests_utils.py 92.34% <0.00%> (+92.34%) ⬆️

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 b0500a5...48aeda5. Read the comment docs.

@yarikoptic yarikoptic merged commit 8f8f0e5 into datalad:maint Jul 26, 2022
@yarikoptic yarikoptic deleted the enh-typing101 branch October 14, 2022 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants