typing and docstring fixes in modules folder and visitor.py

[micpap25]

Summary of changes

Normalize formatting of docstrings (periods, typos, etc.) and apply type hints to functions in modules folder and in visitor.

Edit: Fixes #347

[argus-eye]

Argus review

Auto-review is off for this repo. Tick the box below to run a review on this PR.

• Trigger Argus review

Estimated cost

• Files changed: 1
• Diff lines (±): 56
• Historical avg: ~227.1k tokens · ~$1.18 · across last 3 review(s)

Tip: you can also comment @argus-eye review at any time.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c554f67-dfac-4c45-a6f7-83d02859d97a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[TheGupta2012]

Thanks for starting your contribution journey @micpap25 ! Changes lgtm, as they're mostly related to the docstrs. Can you please make sure that the CI passes and that the CHANGELOG is updated too?

[micpap25]

@TheGupta2012 The reason that mypy was failing was because of the in-progress Pragma changes in the Program class at ast.py. I see there's an open PR regarding Pragma, so maybe I come back to this later?

[micpap25]

@TheGupta2012 I think this PR is ready for merging, in retrospect this should have been part of the same PR as #348 since switching between them is somewhat impacting my workflow as they have a lot of overlap. Let me know what you think.

[ryanhill1]

Thanks for taking this on — the docstring/typing cleanup across modules/ and visitor.py is a real improvement, and mypy src, black, isort and the test suite (648 passed, 3 skipped) are all green.

A few things need addressing before merge. Inline comments are labelled so you can reference them:

• B1–B4 — blocking. B1 breaks import pyqasm on Python 3.10; B2–B4 fail the pylint job in format.yml.
• D1–D4 — malformed or truncated docstrings introduced here.
• R1–R3 — documented return values that don't match the code.
• N1–N7 — nits, take or leave.

One more, no inline anchor: the PR says Fixes #347, but two Returns: None cases in visitor.py are untouched and are exactly the class the issue describes:

• _visit_include (L3287) — returns [include] or []
• _visit_function_call (L2465) — returns tuple[Any | None, list[...]]

Worth folding those in so the issue genuinely closes.

[micpap25]

I believe I have covered everything addressed in the review, @ryanhill1 let me know what you think.

[ryanhill1]

Re-checked everything at bdd18e6. 18 of the 20 threads are genuinely fixed — I've resolved those. Verified rather than eyeballed:

• B1 — built a real 3.10 env: import pyqasm now succeeds. The TypeVar swap is the right call.
• #347 — my AST sweep over visitor.py for documented-None-but-returns-something now comes back empty. _visit_include and _visit_function_call were the two I'd flagged; both are done. The issue genuinely closes.
• R1–R3 — the new "or an empty list if self._check_only is True" phrasing is accurate against the code on all three.
• Tests: 648 passed, 3 skipped. black and isort clean.

Nice work — that's the substance of the review dealt with, including the blocker.

Four small things remain, two of which will red-X the format job, so this isn't quite mergeable yet. All inline below.

F1	mypy regression in base.py — new, and a side-effect of your N2 fix
F2	two new line-too-long in visitor.py
F3	new typo, fuction
F4	two docstrings not yet carrying the _check_only note

Left open deliberately: N1 (the BasisSet narrowing vs. test_rebase.py:266) — you added the type to the docstring but kept the annotation, which is a fair call on a nit; and B2/B3 (cont.), since accept(self, visitor) is still unannotated in all three modules. Neither blocks.

[micpap25]

Tossed in one more fix, while looking at _visit_switch_statement I noticed there was a pylint comment that wasn't doing anything so I moved it to where it should be.

[ryanhill1]

Re-checked at 576fcb0a. F1-F4 are all fixed — resolved those threads.

• F1 — table_data: list[list[Any]] is exactly right; keeps the TypeVar win from N2 and silences the inference.
• F2 — both lines wrapped; pylint on the four touched files is back to 10.00/10.
• F3 — fuction fixed.
• F4 — _visit_switch_statement and _visit_delay_statement both carry the _check_only note now. My consistency sweep over visitor.py finds 0 methods left that read _check_only without documenting it, and 0 docstring/signature return mismatches across all four files.

648 passed, 3 skipped. pylint 10.00/10, black, isort clean. import pyqasm verified on a real 3.10 env.

One regression, from the last commit

576fcb0a ("orphaned type comment") moved the # type: ignore[return] off the def line, which breaks mypy:

src/pyqasm/visitor.py:2797: error: Missing return statement  [return]
Found 1 error in 1 file (checked 37 source files)

The comment wasn't orphaned — its placement was load-bearing. Detail inline. This is the only thing failing CI; put it back and the PR is green everywhere.

Still open, neither blocking, both already discussed: N1 (BasisSet narrowing) and accept(self, visitor) being unannotated in the three modules.

[micpap25]

@ryanhill1 Should be done, fixed B2 / B3.

[ryanhill1]

Re-checked at cdedb54. Toolchain on this head: 727 passed, 3 skipped; black, isort clean; pylint 10.00/10; mypy clean (the only report is the pre-existing tabulate stubs gap, identical on main). # type: ignore[return] verified back on the def line — mypy no longer errors.

Resolved this round

• H1 — the ignore comment is back where it's load-bearing; mypy green. Resolving the thread.
• B2/B3 (cont.) — all three accept implementations now take visitor: QasmVisitor, imported via modules.base, so pylint stays clean. Resolving the thread.

New in the latest commits (J1–J2)

Both docstrings were more accurate before the last edits — the signatures still (correctly) accept Pragma:

J1	visit_statement Args doc dropped | Pragma
J2	visit_basic_block Args doc dropped | Pragma and says list for a Sequence

Missed in my earlier rounds (J3–J5) — not new, flagging for completeness

J3	unroll() kwargs docstring: device_qubits dedented out of the **kwargs nesting, consolidate_qubits nested under it. Heads-up: open #349 edits this same block (it removes device_qubits, which is a loads() kwarg, not an unroll() one) — coordinate to avoid a conflict
J4	_expand_barrier_ranges Args types barrier_qubits as QuantumBarrier; it's list[IndexedIdentifier | Identifier]
J5	_visit_branching_statement: double comma

Still open, unchanged, non-blocking

• N1 — BasisSet narrowing vs test_rebase.py:266; you kept the annotation and documented the type, which I already called a fair resolution. Leaving as-is.

All five J items are one-line docstring fixes — none touch behaviour. Fix those and this is mergeable from my side.

[ryanhill1]

Re-checked at 327e1a7. J3, J4, J5 are done — resolved those threads. J3 resolved itself the nice way: #349 merged into main and your merge brought its version of the unroll() docstring through, so device_qubits is gone and consolidate_qubits sits at the right level. J4 and J5 are exactly right.

Two things still stand between this and green CI:

K1 — J1/J2 landed in the wrong file. The J1, J2 commit edited OpenPulseVisitor.visit_statement / visit_basic_block in src/pyqasm/pulse/visitor.py — same method names, different class. The J1/J2 comments were anchored in src/pyqasm/visitor.py (QasmVisitor, L3414 / L3450), where the docstrings still say Statement without | Pragma. Easy mix-up given the twin names; the original threads stay open.

K2 — the pulse-visitor edit fails black, so the format job is red:

would reformat src/pyqasm/pulse/visitor.py

The continuation lines got 12/8-space indents where black wants 8/4 (diff inline). Running black src fixes it in place.

On the pulse-visitor change itself: no objection to keeping it — the signature widening mirrors the QasmVisitor one and mypy is clean either way. But it wasn't part of any review ask, so if you'd rather keep this PR tight, reverting the two signature changes there and keeping only docstring edits is also fine.

Everything else on this head: 696 passed, 3 skipped; pylint 10.00/10; mypy clean; isort clean. Once J1/J2 land in visitor.py and black is happy, this is mergeable from my side.

[micpap25]

@ryanhill1 Sorry about how many times this has gone back-and-forth, I think this should resolve everything.

[ryanhill1]

Re-checked at f5904b9. K1 and K2 both fixed — the | Pragma docs are in the right file now and black is happy. Resolved J1/J2/K1/K2.

Everything is green on this head for the first time: 696 passed, 3 skipped; black, isort, pylint 10.00/10, mypy all clean; py3.10 import verified.

One last small thing, inline: the updated CHANGELOG entry slightly oversells the pulse/visitor.py coverage. Take the suggestion and this is good to go from my side.

[ryanhill1]

The PR touches two methods in pulse/visitor.py (visit_statement / visit_basic_block), so naming the whole file alongside the four fully-swept modules overstates it a bit:

Suggested change

	- Added / updated type hinting for `base.py`, `qasm2.py`, `qasm3.py`, `visitor.py`, and `pulse/visitor.py` signatures. Fixed grammatical typos in `base.py`, `qasm2.py`, `qasm3.py`, and `visitor.py` docstrings. Fixes incorrect return types in the docstrings of `visitor.py`. ([#346](https://github.com/qBraid/pyqasm/pull/346))
	- Added / updated type hinting for `base.py`, `qasm2.py`, `qasm3.py`, and `visitor.py` signatures, plus the `visit_statement` / `visit_basic_block` signatures in `pulse/visitor.py`. Fixed grammatical typos in `base.py`, `qasm2.py`, `qasm3.py`, and `visitor.py` docstrings. Fixes incorrect return types in the docstrings of `visitor.py`. ([#346](https://github.com/qBraid/pyqasm/pull/346))

[ryanhill1]

Nit, non-blocking either way: the method you just edited still says Returns: None against a list[Statement] signature — the same class of mismatch this PR fixed across visitor.py for #347. Since you're in the file anyway:

Suggested change

	None
	list[Statement]: The list of resulting statements.

(pulse/visitor.py has three more of these in untouched methods — _visit_barrier, _visit_classical_declaration, _visit_function_call — but those can ride with a follow-up issue rather than this PR.)