Skip to content

test: split interface_ipc.py #269

@kevkevinpal

Description

@kevkevinpal

mentioned in: bitcoin#34452

"re: bitcoin#34452 (review)

Could we split out strict move-only changes (formatting and imports are fine) from adjusting method signatures and changing assertions and renaming, etc.

Seems good to make this PR easier to review, but I will say I'm surprised by these comments. I'd agree commit messages should be more accurate and tell reviewers what's changing so they know exactly what to expect and to be surprised if they see any other changes. But the commits themselves seem fine.

In the main commit 3b47cc3 every single line is shown as move-only by git except for import statements and 5 changed signatures:

load_capnp_modules(self)                                  -> load_capnp_modules(config)
make_capnp_init_ctx(self)                                 -> make_capnp_init_ctx(node, capnp_modules)
parse_and_deserialize_block(self, block_template, ctx)    -> get_block(block_template, ctx)
get_coinbase_raw_tx(self, block_template, ctx)            -> get_coinbase_raw_tx(block_template, ctx)
parse_and_deserialize_coinbase(self, block_template, ctx) -> get_coinbase_tx(block_template, ctx)

So reviewing the commit feels completely trivial to me, it's just a matter of making sure all the lines are move only, and the only lines that aren't move-only are the simple substitutions above. I think I only spent 1-2 minutes reviewing it myself.

That said, it would be nice to split up, maybe by using @staticmethod to get rid of the self arguments and by defining global aliases load_capnp_modules = IPCInterfaceTest.load_capnp_modules so the signatures can change once in an initial commit and the followup commit can be 100% move-only.

" - ryanofsky

Metadata

Metadata

Assignees

No one assigned

    Labels

    followupThis is a possible follow-up someone can work onneed to confirmNeed to confirm if this is still needed or already done

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions