Skip to content

fix: ExternalRunnerTestInfo.type matches constructor#1318

Open
lf- wants to merge 1 commit into
facebook:mainfrom
MercuryTechnologies:jade/push-tnypuvqwqqno
Open

fix: ExternalRunnerTestInfo.type matches constructor#1318
lf- wants to merge 1 commit into
facebook:mainfrom
MercuryTechnologies:jade/push-tnypuvqwqqno

Conversation

@lf-
Copy link
Copy Markdown
Contributor

@lf- lf- commented May 12, 2026

I found a really funny thing in the ExternalRunnerTestInfo docs: the type field is shown as .test_type, not .type (!).

This is because it is .test_type as a field and type in the constructor. This is super confusing, let's unify the field (that I doubt anyone accesses) with the constructor so the docs make more sense.

I found a really funny thing in the ExternalRunnerTestInfo docs:
the `type` field is shown as `.test_type`, not `.type` (!).

This is because it *is* `.test_type` as a field and `type` in the
constructor. This is super confusing, let's unify the field (that I
doubt anyone accesses) with the constructor so the docs make more sense.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 12, 2026
@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync Bot commented May 12, 2026

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D104869246. (Because this pull request was imported automatically, there will not be any future comments.)

@cormacrelf
Copy link
Copy Markdown
Contributor

I have definitely experienced this issue before. 👍

@JakobDegen
Copy link
Copy Markdown
Contributor

(that I doubt anyone accesses)

Heh, grep in fbsource gives me 23 results.

But also, .type is a starlark builtin for getting the name, we definitely don't want to overload that. Can we go the other way instead? Add test_type as a thing we accept in the constructor, I'll volunteer to (dispath an AI to) codemod our constructors, and then we go from there? (I've also got a diff going that makes all the existing parameters named-only, that should help with that)

@lf-
Copy link
Copy Markdown
Contributor Author

lf- commented May 18, 2026

But also, .type is a starlark builtin for getting the name, we definitely don't want to overload that.

news to me! that's unfortunate, and I agree this would be a flawed approach then.

Can we go the other way instead? Add test_type as a thing we accept in the constructor, I'll volunteer to (dispath an AI to) codemod our constructors, and then we go from there?

Sounds great to me!

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants