Add pyrefly type checking to moz-phab test suite
Categories
(Conduit :: moz-phab, enhancement, P3)
Tracking
(Not tracked)
People
(Reporter: sheehan, Assigned: sheehan)
Details
(Keywords: leave-open)
Attachments
(30 files)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
We have type annotation enforcement via the ANN* rules in ruff check, however ruff doesn't check that type hints are correct. We should add pyrefly to the moz-phab dev dependencies and add a test that enforces type hints are correct through the repo.
| Assignee | ||
Comment 1•1 month ago
|
||
check_output returned a list, a string or raw bytes depending on its split and
expect_binary flags, so callers had to work with the union of all three. Each
helper is now a shared implementation plus one function per return type --
check_output, check_output_text and check_output_binary, and the same shape for
git_out and the jj helpers -- so callers say which form they want.
| Assignee | ||
Comment 2•1 month ago
|
||
hg_out returned a list, a string, raw bytes or nothing depending on its split,
expect_binary and capture flags. hg_run now holds the implementation, with
hg, hg_out, hg_out_text and hg_out_binary naming the form they return, and
hg_log/hg_log_text sharing the command they build.
| Assignee | ||
Comment 3•1 month ago
|
||
split_lines returned a bytes | str list whichever it was given, which its only
caller then passed on as strings. Text and binary content now have a function each,
so the element type follows from the name.
| Assignee | ||
Comment 4•1 month ago
|
||
The unannotated stubs read as taking and returning None, so every typed override
in Git, Mercurial and Jujutsu looked inconsistent with the base class.
commit_stack also names the single argument its callers pass.
| Assignee | ||
Comment 5•1 month ago
|
||
_git_get_children returns the list of "parent child child" lines its docstring
describes, _cat_file and hg_cat return bytes, _get_parent always finds a node,
Git.commit takes the epoch apply_patch passes it, and hgclient only exists as
hglib.client.hgclient. Diff.set_change_kind compares its kind against single
letters rather than a Kind.
| Assignee | ||
Comment 6•1 month ago
|
||
edit_revision accepts a revision id or a PHID and a tri-state wip because that
is what Commit carries, and its differential.revision.edit arguments hold more
than the transaction list they are seeded with. call returns whatever the method
it invoked returns, which is a raw string for differential.getrawdiff.
| Assignee | ||
Comment 7•1 month ago
|
||
stop_logging iterates the list to remove and close them, and it holds both a
stream and a rotating file handler.
| Assignee | ||
Comment 8•1 month ago
|
||
The docstring-only stubs silently returned None where they declare a bool, a
Diff, a node or a list of commits. Every backend implements these, except that
Git has no untracked, so that one keeps its previously implicit empty default.
| Assignee | ||
Comment 9•1 month ago
|
||
args started out as None, so every self.args.<option> access in the backends
was an attribute lookup on None, and vcs was untyped for the same reason.
phab_vcs caches through a local so its str return is provable.
| Assignee | ||
Comment 10•1 month ago
|
||
Both get_repo_head_branch implementations read the _phab_repo cache field
directly, which is None until something else populates it through the property. A
repository without a callsign reached repository.search with None, and
is_cinnabar_required only existed on the Git based backends even though
build_creation_method reads it from any repository.
| Assignee | ||
Comment 11•1 month ago
|
||
repo was an untyped None, so every conduit.repo.<attr> read looked like an
attribute lookup on None. It is now a property backed by an optional field that
raises a clear error when the repository has not been set yet, and the tests reuse
Repository for their stand-in rather than a bare class.
| Assignee | ||
Comment 12•1 month ago
|
||
get_revisions rebound its ids parameter from list[int] to list[str], both
lookups let an empty ids list past their argument check and then iterated None,
and edit_revision built its constraint in a dict it splatted. The retry loop
gains an explicit else, so the response is always bound before it is read.
| Assignee | ||
Comment 13•1 month ago
|
||
Git._commit_tree needs parent, author_date and tree_hash as strings, which
the backends that support them always provide; those without them leave the field
empty, which reads the same as the old None at the one place that tests it. The
epoch is built as an int, since that is what Phabricator receives.
| Assignee | ||
Comment 14•1 month ago
|
||
The hunk header and its lines were collected in an untyped dict and splatted into
Hunk(**hunk), and a header that failed to match raised AttributeError on None
from the four match.group calls that followed. The diff ids Phabricator returns
start empty rather than as None.
| Assignee | ||
Comment 15•1 month ago
|
||
set_as_binary stores its bodies for upload, where they are hashed and base64
encoded, so they have to be bytes. Git._parse_diff_change and
Mercurial._get_file_meta each reused one variable for the raw blob and its
decoded text, and the git side could hand a decoded str to one side of a binary
change.
| Assignee | ||
Comment 16•1 month ago
|
||
The dicts mixed filenames with the handler to call, so c["fn"] looked like it
could be either a string or a method. get_diff keeps the parent it just resolved
in a local, which is what the handlers need.
| Assignee | ||
Comment 17•1 month ago
|
||
_get_successor declares a (rev, node) tuple and its caller unpacks one, but it
returned the list str.split produced. _get_config_options returns (name, value) pairs, so _get_extensions has been logging an empty list in safe mode,
and the revset, bookmark and obsstore paths are filled in before use.
| Assignee | ||
Comment 18•1 month ago
|
||
commit_stack tracked whether it had started through the child list rather than the
first node, is_descendant and uplift_commits subscript the revset that
set_args fills in, and finalize checked out the branch without the guard
cleanup uses. rebase_commit takes the Commits the base class and its only
caller pass, and get_public_node drops a cache that _git_to_hg already provides.
| Assignee | ||
Comment 19•1 month ago
|
||
Jujutsu has no git attribute, so reporting an invalid email raised
AttributeError instead of naming the address. apply_patch receives an optional
author and creation date, which patch leaves unset when a diff has no commit
attachment, and passed both straight into jj metaedit.
| Assignee | ||
Comment 20•1 month ago
|
||
prompt bound options_map only when it had options, replace_reviewers rebound
its str parameter to a list of lines, and check_call_by_line read
process.stdout without stating that the pipe is always requested.
| Assignee | ||
Comment 21•1 month ago
|
||
_build_request passed an unset bmo_url straight to urljoin, _sanitise_req
assumed the request always carries headers, and BMOAPIError was handed an
exception where it declares a string.
| Assignee | ||
Comment 22•1 month ago
|
||
install_scripts is optional, and Path(None) would have raised TypeError during
a self-update check.
| Assignee | ||
Comment 23•1 month ago
|
||
patch tested args.apply_to where the branches above had already decided whether
there is a base node to check. reorganise annotates the collections it builds,
collects the revision ids after the check that they are all present, and leaves text
unlinked for repositories with no Bugzilla, rather than linking to
None/show_bug.cgi.
| Assignee | ||
Comment 24•1 month ago
|
||
Commit.rev_id is optional, so the AI review request list and its result lookup
were both typed as possibly holding None keys, and the uplift assessment URLs were
built from the tip commit whether or not it had an id.
| Assignee | ||
Comment 25•1 month ago
|
||
The fakes stand in for parsed command line arguments, so they belong to the same
type as the real thing, as test_submit.py already did.
| Assignee | ||
Comment 26•1 month ago
|
||
The fake stands in for a repository in local_uplift_if_possible and
update_commits_for_uplift, so it now subclasses Repository, seeds the _phid
cache the phid property reads, and returns the commits from uplift_commits.
| Assignee | ||
Comment 27•1 month ago
|
||
test_get_diff_by_id drove the function through an args dict, so the "no diff" case
actually exercised the not-found branch; it now covers the three branches directly.
The reorganise tests pass a repository stand-in rather than None, and HTTPError
and MaxRetryError get the header and reason objects they declare.
| Assignee | ||
Comment 28•1 month ago
|
||
test_is_descendant and test_patch asserted on the raised error after the call
that raises, so the assertions never executed, and both read the wrong attribute off
ExceptionInfo. The uplift and user data tests check their optional values before
using them, so a failure surfaces as the assertion rather than an AttributeError.
| Assignee | ||
Comment 29•1 month ago
|
||
hglib spawns the command server from hglib.HGPATH, so pointing it at a
configured hg means setting that global. hglib ships no type information, so its
default value gives the global a literal type that no other path is assignable to,
which the cast states explicitly at the assignment.
| Assignee | ||
Comment 30•1 month ago
|
||
pyrefly joins the development dependencies, pyrefly.toml configures it against
the oldest supported interpreter, and test_pyrefly fails on any type error that is
not recorded in pyrefly-baseline.json, which is empty now that the tree checks
clean. Regenerate the baseline with uv run pyrefly check --baseline pyrefly-baseline.json --update-baseline from the repository root, since it stores
repository-relative paths.
Comment 31•1 month ago
|
||
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/4791143f9f41f28881addb7996422872b5c37342
[main] vcs: name the command output helpers after what they return (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/30ab230df1d869c2d5e39547e128d638d2cf2e6b
[main] mercurial: name the hg output helpers after what they return (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/1d0e68bfab3efac62b35347cbc229e07ec5fb8cf
[main] helpers: split the text and binary line splitting (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/6ce48d22c96e75fe56b9a0d2156da6a172eb6525
[main] repository: annotate the base methods the backends override (Bug 2064223) r=shtrom
| Assignee | ||
Updated•1 month ago
|
Comment 32•1 month ago
|
||
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/e0ee61488e9f3306600854aa1a0e52546bfb0ef8
[main] vcs: correct the annotations that disagree with the code (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/eb654edf01dfeebc96aa3d64f206a209e23507a5
[main] conduit: correct the annotations on the revision helpers (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/2a6c4287af534543f0188114dc9e12a1d83721f7
[main] logger: annotate the list of installed handlers (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/ea0dfdfa68b68ba23be81a88feba9c0a9e1e176a
[main] repository: raise NotImplementedError from the base stubs (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/9487a71c90d2b9c575dd1b7f51826daee3343a61
[main] repository: give the command line and VCS attributes real values (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/552fcb6e8ceb9fe5a82f410b036744109dc01126
[main] repository: cover the repository data lookups in the base class (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/b220c0c2f18d15411517ccfbc267de02bb49e08c
[main] conduit: hold the repository in a typed property (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/e3998f341024cb3e44235ffc7af5894fda6f98b6
[main] conduit: type the revision and diff lookups (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/b5feecd4e70aad566ea4b85a5115bb84bc656bfd
[main] commits: type the fields the backends always fill in (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/fad9a6e5bee495f2e9949e393a91ae6e2ef9db24
[main] diff: build hunks from typed values and reject bad headers (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/ce797c44884a8b1559fddb7c182424a9a20ea3e6
[main] diff: keep binary file bodies as bytes (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/001a72ebbba0177853acd06b0a52a1d1af2d9c78
[main] mercurial: collect file changes in a frozen dataclass (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/9d040dfaa8db0a92f06ef5bcb537c322c2c3e8a9
[main] mercurial: fix the successor tuple and the state it tracks (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/da0e0b8745e42631191efc4bf27a999b76841e7e
[main] git: type the commit range, branch and rebase arguments (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/d75215e2ca7e90fa157d17b071777ed73e00d4f0
[main] jujutsu: fix the email error message and the patch metadata (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/58d8112d3afbd5d5c210ea4e14321b06cddbaffc
[main] helpers: initialise the values that only one branch assigns (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/564749319dc219e60d89baa9e7c969a57b8db37a
[main] bmo: handle a missing Bugzilla URL and header dict (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/f85bced2b21043cccb067ee071b828e7909a5756
[main] updater: fail clearly when the user scripts directory is unknown (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/8ea88b1e3bf8073949bc9515a9a0a67318fd75ae
[main] patch, reorganise: handle the optional values in the command flow (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/60fc6b72bc21a59a6ad0f1fe5d441b9b304e5d23
[main] submit, uplift: key the follow-up work on the revision ids (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/3e870c740848df671d79181c4294092d7be53e5f
[main] tests: base the fake Args classes on argparse.Namespace (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/ca71841f45c4e7e8df9b0cba8710f2aa68dd6ca7
[main] tests: base the uplift fake repo on Repository (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/175427aadcbec9d51ff4d1338e16bd542ee8d123
[main] tests: pass the types the code under test declares (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/694f0b75308588acecceffc903c1294921339a6b
[main] tests: run the assertions that sit inside pytest.raises blocks (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/fcdc319440bf141f9fdfbb0ef80c2ab0ca0dbfcc
[main] mercurial: cast the hg path assigned to the hglib global (Bug 2064223) r=shtrom
Authored by https://github.com/cgsheeh
https://github.com/mozilla-conduit/review/commit/111628d7a2995af41fb71ac273cc73162e47b361
[main] tests: check types as part of the suite (Bug 2064223) r=shtrom
| Assignee | ||
Updated•1 month ago
|
Description
•