Skip to content

Add mypy check to github actions - #177

Merged
avara1986 merged 2 commits into
python-microservices:masterfrom
ooigavin:feature/add-mypy
Oct 8, 2020
Merged

Add mypy check to github actions#177
avara1986 merged 2 commits into
python-microservices:masterfrom
ooigavin:feature/add-mypy

Conversation

@ooigavin

@ooigavin ooigavin commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

Fixes # 166

Changes proposed in this PR:

  • Add type checking in github actions using mypy
  • Improve type hinting in the code
@coveralls

coveralls commented Oct 6, 2020

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 942

  • 16 of 16 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.156%

Totals Coverage Status
Change from base Build 939: 0.0%
Covered Lines: 1644
Relevant Lines: 1658

💛 - Coveralls
@ooigavin

ooigavin commented Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

Hi @avara1986 I have added the mypy checks into github actions, still working on reducing the errors & adding more type hints. However I have some questions regarding the pipfile and the lock file. Not too familiar with it as I normally use pip & virtualenv in my own projects. But I do understand the concept of a lock file from other package managers like yarn.

When I generated the lockfile I see that not only did i add the mypy dependency, I also upgraded the minor versions of other dependencies (which should be non-breaking). But what I would like to ask is what the markers do in the lockfile, cause I noticed that regenerating the lockfile added a few of these markers.

My python env is 3.7.5

@avara1986

Copy link
Copy Markdown
Member

Hi @ooigavin,

It's normal that if you run pipenv lock you see new changes (not only your changes). His behavior is about this definition:

lightstep>=4.4.8

The most of pyms dependencies are "greater or equeal to [version]" and each time you run pipenv pipenv lock if a package has a new version, the pipfile.lock will be updated.

The new markers means that this library updated the Python version dependencie (python_version < '3.8', python_version > '3.5', etc)

I hope this help 😄

@ooigavin

ooigavin commented Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

Goddit, thanks! one other thing I wanted to check, I am used to creating small commits for each change i make. However sometimes I think maintainers prefer PRs to contain a single commit? Not sure what is the convention for this project.

@avara1986

Copy link
Copy Markdown
Member

IMHO, i like both. One big PR or a lot of small PRs ��� BUT, if you are playing the Hacktoberfest I think a lot of small PRs with one or two lines updated could be considered as SPAM 👍

@ooigavin

ooigavin commented Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

oh what i meant is in a single PR, is it alright if it contains many commits, or should it just contain one commit containing all the changes? Only intend to submit a single PR for this

@avara1986

Copy link
Copy Markdown
Member

I read that i want sorry....:joy: No, you can do all commits that you want, It's ok 1 or 1000 :+1:

@ooigavin
ooigavin marked this pull request as ready for review October 7, 2020 01:56
@ooigavin

ooigavin commented Oct 7, 2020

Copy link
Copy Markdown
Contributor Author

Have added more type hints and reduced some of the mypy errors, left some whr I wasnt too sure about the typing alone.

@avara1986

avara1986 commented Oct 7, 2020

Copy link
Copy Markdown
Member

Great PR @ooigavin ! is ready to review and merge or do you want to add more commits? 👍

@ooigavin

ooigavin commented Oct 8, 2020

Copy link
Copy Markdown
Contributor Author

I've looked thru most of the codebase, and added the types whr I was confident to add them. I'm ready to merge, unless you need me to make any changes?

@avara1986
avara1986 self-requested a review October 8, 2020 21:43
@avara1986 avara1986 linked an issue Oct 8, 2020 that may be closed by this pull request
@avara1986
avara1986 merged commit aefb690 into python-microservices:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants