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

Fix missing field for tffmt in lint #13355

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Oct 25, 2021

#13301 changed TffmtRequest's inheritance of StyleRequest, but didn't add an abstract field. mypy didn't flag this, likely because our use of @union obscures the constructor call that might have triggered the error: see https://mypy.readthedocs.io/en/stable/class_basics.html#abstract-base-classes-and-multiple-inheritance.

It's possible that #12577 would improve the situation here, but it doesn't seem worth adding heavier integration tests (which would invoke the entire lint and fmt goals) just to catch this kind of case.

[ci skip-rust]
[ci skip-build-wheels]

@stuhood stuhood added this to the 2.8.x milestone Oct 25, 2021
@stuhood stuhood enabled auto-merge (squash) October 25, 2021 23:22
@stuhood stuhood changed the title tffmt broke in lint due to a missing field Fix missing field for tffmt in lint Oct 25, 2021
@stuhood stuhood disabled auto-merge October 25, 2021 23:39
[ci skip-rust]

[ci skip-build-wheels]
@stuhood stuhood enabled auto-merge (squash) October 25, 2021 23:42
@stuhood stuhood merged commit f57e7f0 into pantsbuild:main Oct 26, 2021
@asherf
Copy link
Member

asherf commented Oct 26, 2021

yay! thanks!

@stuhood stuhood deleted the stuhood/fix-tffmt-lint branch October 26, 2021 03:10
stuhood added a commit to stuhood/pants that referenced this pull request Oct 26, 2021
pantsbuild#13301 changed `TffmtRequest`'s inheritance of `StyleRequest`, but didn't add an abstract field. `mypy` didn't flag this, likely because our use of `@union` obscures the constructor call that might have triggered the error: see https://mypy.readthedocs.io/en/stable/class_basics.html#abstract-base-classes-and-multiple-inheritance. 

It's possible that pantsbuild#12577 would improve the situation here, but it doesn't seem worth adding heavier integration tests (which would invoke the entire `lint` and `fmt` goals) just to catch this kind of case.

[ci skip-rust]
[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Oct 26, 2021
#13301 changed `TffmtRequest`'s inheritance of `StyleRequest`, but didn't add an abstract field. `mypy` didn't flag this, likely because our use of `@union` obscures the constructor call that might have triggered the error: see https://mypy.readthedocs.io/en/stable/class_basics.html#abstract-base-classes-and-multiple-inheritance. 

It's possible that #12577 would improve the situation here, but it doesn't seem worth adding heavier integration tests (which would invoke the entire `lint` and `fmt` goals) just to catch this kind of case.

[ci skip-rust]
[ci skip-build-wheels]
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