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

Implement typings in conda-smithy #1957

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ninetteadhikari
Copy link

@ninetteadhikari ninetteadhikari commented Jun 20, 2024

Introduction

This work is done according to "Milestone 2: Implement typings in conda-smithy" as stated in the Scope of Work with Sovereign Tech Fund (STF) (https://www.sovereigntechfund.de/).

This PR addresses the typing request from this issue

Setup

Setup details are here: https://github.com/neighbourhoodie/conda-smithy/wiki

Steps followed to add type

As suggested in the ticket in the conda-smithy repo, MonkeyType is used to generate type hints for the python code.

Generate type hints

  1. Install MonkeyType
pip install MonkeyType
  1. Run tests using MonkeyType. This will dump call traces into a SQLite database in the file monkeytype.sqlite3 in the current working directory.
monkeytype run -m pytest
  1. Make a separate folder called stubs to save the type hint files
mkdir stubs
  1. Generate stub files for each of the modules found in the SQLite database
monkeytype list-modules | xargs -n1 -I{} sh -c 'monkeytype stub {} > stubs/{}.pyi'

The above command will create stub files in the stubs folder. Here's an example of what the content of a stub file will look like. It gives suggestions for types.

def rotate_anaconda_token(
    user: str,
    project: str,
    feedstock_config_path: Optional[str],
    drone: bool = ...,
    circle: bool = ...,
    travis: bool = ...,
    azure: bool = ...,
    appveyor: bool = ...,
    github_actions: bool = ...,
    token_name: str = ...,
    drone_endpoints: List[str] = ...
): ...
  1. The suggestions generated in the stub file were used make changes to the actual python code.

Checklist

  • Added a news entry
  • Ran monkeytypes to get type suggestions
  • Add type to conda_smithy modules
  • Add type to tests
  • Add documentation on using Monkeytype
  • Add mypy for type checks

Note:

Please note that there are some errors thrown by mypy that couldn't resolved at the moment and there is more room for further type improvements there.

@ninetteadhikari ninetteadhikari requested a review from a team as a code owner June 20, 2024 15:00
@ninetteadhikari ninetteadhikari force-pushed the m2-add-type branch 2 times, most recently from 82d1fb1 to b2a66d4 Compare June 27, 2024 19:29
@beckermr
Copy link
Member

pre-commit.ci autofix

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I'm ok to merge this but I'd rather the type checks be optional and simply reported to people on PRs so they can fix them if they like.

@ninetteadhikari
Copy link
Author

ninetteadhikari commented Aug 28, 2024

I'm ok to merge this but I'd rather the type checks be optional and simply reported to people on PRs so they can fix them if they like.

Hi @beckermr thanks for your response! I've rebased the PR and made type check optional bafe408. let me know if this is okay or if you have any other feedback:)

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Thank you!

Any comments from any one else on @conda-forge/core?

@xhochy
Copy link
Member

xhochy commented Sep 4, 2024

I'm ok to merge this but I'd rather the type checks be optional and simply reported to people on PRs so they can fix them if they like.

I feel that if you add types, you should also make use of them and have them checked. Otherwise, it might be even better to not have them as otherwise you will be making false assumptions.

@beckermr
Copy link
Member

beckermr commented Sep 4, 2024

OK @xhochy, then I'd be in favor of removing the types all together.

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