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

Add a lot of runtime defaults to stubs with stubdefaulter #2327

Merged
merged 7 commits into from
Aug 7, 2024

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Aug 7, 2024

You can do the same with:

# Install
pip install stubdefaulter no_implicit_optional

# Run
PYTHONPATH='PATH_TO_YOUR/site-packages/django' stubdefaulter -p django-stubs
no_implicit_optional django-stubs --use-union-or

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

This looks great. 👍

Not going to go through all of it but instead assume that stubdefaulter behaves properly. We can improve any inconsistencies/incorrect stuff over time.

One question; can we add stubdefaulter to CI to keep defaults populated for new changes?

@sobolevn
Copy link
Member Author

sobolevn commented Aug 7, 2024

One question; can we add stubdefaulter to CI to keep defaults populated for new changes?

typeshed does this! I will add it in the next PR, so it won't be hidden among 3000 lines diff 😆

@flaeppe
Copy link
Member

flaeppe commented Aug 7, 2024

Sounds great!

I suppose one way is to check git for changes after running the command. Just want to drop off a run I've used for these cases a couple of times in other CI's:

        run: |
          set -e
          <install deps>
          git add -A
          <run command>
          if [ -n "$(git status --porcelain -- <path>)" ]; then
            echo "::error::Build result differs. Run '<run command>' then commit and push any changes"
            git status --porcelain -- <path>
            exit 1
          fi

Note: replace <> marked parts with stuff for stubdefaulter

@sobolevn
Copy link
Member Author

sobolevn commented Aug 7, 2024

We have a huge list of fixed problems in stubtest:

 note: unused allowlist entry django.contrib.gis.db.models.ForeignObjectRel.__init__
note: unused allowlist entry django.contrib.gis.db.models.ManyToOneRel.__init__
note: unused allowlist entry django.contrib.gis.db.models.OneToOneRel.__init__
note: unused allowlist entry django.contrib.gis.db.models.Q.resolve_expression
note: unused allowlist entry django.contrib.gis.forms.Widget.subwidgets
note: unused allowlist entry django.core.mail.SafeMIMEMultipart.__init__
note: unused allowlist entry django.core.mail.SafeMIMEText.__init__
note: unused allowlist entry django.core.mail.message.SafeMIMEMultipart.__init__
note: unused allowlist entry django.core.mail.message.SafeMIMEText.__init__
note: unused allowlist entry django.db.migrations.graph.MigrationGraph.make_state
note: unused allowlist entry django.db.models.ForeignObjectRel.__init__
note: unused allowlist entry django.db.models.ManyToOneRel.__init__
note: unused allowlist entry django.db.models.OneToOneRel.__init__
note: unused allowlist entry django.db.models.Q.resolve_expression
note: unused allowlist entry django.db.models.fields.related.ForeignObjectRel.__init__
note: unused allowlist entry django.db.models.fields.related.ManyToOneRel.__init__
note: unused allowlist entry django.db.models.fields.related.OneToOneRel.__init__
note: unused allowlist entry django.db.models.fields.reverse_related.ForeignObjectRel.__init__
note: unused allowlist entry django.db.models.fields.reverse_related.ManyToOneRel.__init__
note: unused allowlist entry django.db.models.fields.reverse_related.OneToOneRel.__init__
note: unused allowlist entry django.db.models.query.RawQuerySet.__init__
note: unused allowlist entry django.db.models.query_utils.Q.resolve_expression
note: unused allowlist entry django.forms.Widget.subwidgets
note: unused allowlist entry django.forms.widgets.ChoiceWidget.subwidgets
note: unused allowlist entry django.forms.widgets.Widget.subwidgets
note: unused allowlist entry django.template.EngineHandler.__init__
note: unused allowlist entry django.template.utils.EngineHandler.__init__
note: unused allowlist entry django.templatetags.i18n.BlockTranslateNode.__init__
note: unused allowlist entry django.templatetags.static.PrefixNode.__init__
note: unused allowlist entry django.templatetags.static.StaticNode.__init__
Found 30 errors (checked 880 modules)

sobolevn added a commit to sobolevn/stubdefaulter that referenced this pull request Aug 7, 2024
We want to integrate `stubdefaulter` to `django-stubs`: typeddjango/django-stubs#2327

Later we also want to add `stubdefaulter` to our CI, so it can check that no missing defaults are added. 

`--check` would help us to fail the CI. We would use both `--exit-zero` and `--check`, because there would be errors when importing django modules, we can't fix it from our side. So, we need them both to work correctly. Errors will be ignored with `--exit-zero` while `--check` will return `2` if there are any new changes.
@sobolevn
Copy link
Member Author

sobolevn commented Aug 7, 2024

I suppose one way is to check git for changes after running the command.

@flaeppe I have a better idea :)

JelleZijlstra/stubdefaulter#96

@flaeppe
Copy link
Member

flaeppe commented Aug 7, 2024

Yeah that's even better

@sobolevn sobolevn merged commit 9f1401b into master Aug 7, 2024
36 checks passed
@sobolevn sobolevn deleted the stubdefaulter-second-iteration branch August 7, 2024 08:45
JelleZijlstra pushed a commit to JelleZijlstra/stubdefaulter that referenced this pull request Aug 7, 2024
We want to integrate `stubdefaulter` to `django-stubs`: typeddjango/django-stubs#2327

Later we also want to add `stubdefaulter` to our CI, so it can check that no missing defaults are added. 

`--check` would help us to fail the CI. We would use both `--exit-zero` and `--check`, because there would be errors when importing django modules, we can't fix it from our side. So, we need them both to work correctly. Errors will be ignored with `--exit-zero` while `--check` will return `2` if there are any new changes.
@adamchainz
Copy link
Contributor

Very nice, thank you @sobolevn .

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

Successfully merging this pull request may close these issues.

3 participants