Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Correctly pull twisted trunk when triggered overnight and ignore known type error #16115

Merged
merged 6 commits into from
Aug 15, 2023

Conversation

DMRobertson
Copy link
Contributor

See the discussion in #15097.

Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Any way to test this? Seems reasonable...

Comment on lines +101 to +103
# type-ignore: This is an error in Twisted's annotations. See
# https://github.com/twisted/twisted/issues/11812 and /11813 .
factory = manhole_ssh.ConchFactory(portal.Portal(rlm, [checker])) # type: ignore[arg-type]
Copy link
Contributor Author

@DMRobertson DMRobertson Aug 15, 2023

Choose a reason for hiding this comment

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

Urgh, the error is only introduced in twisted trunk, so adding this triggers an unused-ignore error on the current build.

Maybe I can use a cast instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted just to tell mypy to relax the unused-ignore check for this file. (Simpler, and I'm pretty sure it can complain about a redundant cast sometimes.)

Copy link
Member

Choose a reason for hiding this comment

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

Not a huge fan of adding an ignored file, can we do type: ignore[arg-type,<whatever the code is for an unneeded ignore>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure it has an error code:

synapse/util/manhole.py:103: error: Unused "type: ignore" comment

Maybe I can just use a naked # type[ignore]?

Copy link
Member

Choose a reason for hiding this comment

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

Worth a try, if not :shipit:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No luck, I'm afraid. 🚢 ping it

David Robertson added 4 commits August 15, 2023 15:15
One day I'm gonna write a linter for this, and write a workflow to run
the linter on all workflows, including the workflow itself.
@DMRobertson
Copy link
Contributor Author

I can test this by temporarily forcing this branch to run the twisted-trunk job.

It looks like the changes were successful. They point to new typechecking errors: https://github.com/matrix-org/synapse/actions/runs/5868230260/job/15910583758?pr=16115, which I would guess are mostly due to twisted/twisted#11770

@clokep
Copy link
Member

clokep commented Aug 15, 2023

It looks like the changes were successful. They point to new typechecking errors: matrix-org/synapse/actions/runs/5868230260/job/15910583758?pr=16115, which I would guess are mostly due to twisted/twisted#11770

Should we fix that here or merge this and do a follow-up?

@DMRobertson
Copy link
Contributor Author

DMRobertson commented Aug 15, 2023

Should we fix that here or merge this and do a follow-up?

My vote is the latter: merge and follow-up.

@DMRobertson
Copy link
Contributor Author

(Will let CI run, then revert da3f079 (#16115))

@DMRobertson
Copy link
Contributor Author

CI against the main release of twisted seemed fine.

@DMRobertson DMRobertson marked this pull request as ready for review August 15, 2023 15:30
@DMRobertson DMRobertson requested a review from a team as a code owner August 15, 2023 15:30
@DMRobertson DMRobertson enabled auto-merge (squash) August 15, 2023 15:31
@DMRobertson DMRobertson enabled auto-merge (squash) August 15, 2023 15:39
@DMRobertson DMRobertson changed the title Attempt to fix twisted trunk Correctly pull twisted trunk when triggered overnight; ignore known type error Aug 15, 2023
@DMRobertson DMRobertson changed the title Correctly pull twisted trunk when triggered overnight; ignore known type error Correctly pull twisted trunk when triggered overnight and ignore known type error Aug 15, 2023
@DMRobertson DMRobertson merged commit 47c629b into develop Aug 15, 2023
37 checks passed
@DMRobertson DMRobertson deleted the dmr/fix-twisted-trunk branch August 15, 2023 16:07
H-Shay pushed a commit that referenced this pull request Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants