Refactor: separate function of WM_OT_batch_rename #119160

Christoph Lendenfeld · 2024-03-07T13:01:26+01:00

2024-03-07 13:01:26 +01:00

No functional changes.

The function _data_from_context of the WM_OT_batch_rename operator
was used to do two completely different things.
It either got data or data_type depending on the flags passed in.
To simplify the code a new function has been created
_data_type_from_context which only ever returns data_type

This PR was split off from #118967: Refactor: WM_OT_batch_rename
The long term goal is to refactor the code so #115185: Fix #115166: Batch Rename Action Clips error can land

No functional changes. The function `_data_from_context` of the `WM_OT_batch_rename` operator was used to do two completely different things. It either got `data` or `data_type` depending on the flags passed in. To simplify the code a new function has been created `_data_type_from_context` which only ever returns `data_type` ------ This PR was split off from [#118967: Refactor: WM_OT_batch_rename](https://projects.blender.org/blender/blender/pulls/118967) The long term goal is to refactor the code so [#115185: Fix #115166: Batch Rename Action Clips error](https://projects.blender.org/blender/blender/pulls/115185) can land

Christoph Lendenfeld added 2 commits 2024-03-07 13:01:35 +01:00

0f0525f0db split off function

c737177fb9 fix double variable assignment

Christoph Lendenfeld requested review from Campbell Barton 2024-03-07 13:12:18 +01:00

Christoph Lendenfeld added the

Module

User Interface

label 2024-03-07 13:12:34 +01:00

Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-03-07 13:12:49 +01:00

Campbell Barton commented

2024-03-14 01:00:15 +01:00

The new logic had the down-side it's duplicating code-paths in a way that can diverge in a non-obvious way, in fact I wouldn't know from looking at this if this doesn't cause them to diverge. Debugging this kind of issue is time consuming and makes updating the function less straightforward.

However I can see the motivation for the refactor as the current logic mixes a "poll" with an "exec" function (using operator terminology) for many different contexts/types. I think it would be better to isolate handling of each context so the logic can't diverge.

The new logic had the down-side it's duplicating code-paths in a way that can diverge in a non-obvious way, in fact I wouldn't know from looking at this if this doesn't cause them to diverge. Debugging this kind of issue is time consuming and makes updating the function less straightforward. However I can see the motivation for the refactor as the current logic mixes a "poll" with an "exec" function (using operator terminology) for many different contexts/types. I think it would be better to isolate handling of each context so the logic can't diverge.

Sybren A. Stüvel commented

2024-03-18 12:27:30 +01:00

The new logic had the down-side it's duplicating code-paths in a way that can diverge in a non-obvious way

That is a good point.

However:

in fact I wouldn't know from looking at this if this doesn't cause them to diverge. Debugging this kind of issue is time consuming and makes updating the function less straightforward.

This I feel reflects as much onto the existing code as it does on the new code. Having a boolean parameter that changes what a function does and what it returns, is intrinsically not straightforward. Understanding this code is already time consuming.

The anonymous tuples also make things harder to follow, as there is no documentation what the individual fields mean, and the deep nesting in places where they are being generated makes things even more complex.

To address Campbell's concern, maybe the code path that currently returns the 'rename info tuple' should just return a class (instance). And that class (instance) can then have functions for returning the 'data type' and the 'rename info tuple', so that those two things are closely tied.

And of course the 'rename info tuple' should (IMO) be replaced by something more self-documenting; at the minimum a NamedTuple, but I think a dataclass would be more appropriate.

> The new logic had the down-side it's duplicating code-paths in a way that can diverge in a non-obvious way That is a good point. However: > in fact I wouldn't know from looking at this if this doesn't cause them to diverge. Debugging this kind of issue is time consuming and makes updating the function less straightforward. This I feel reflects as much onto the existing code as it does on the new code. Having a boolean parameter that changes what a function does and what it returns, is intrinsically not straightforward. Understanding this code is already time consuming. The anonymous tuples also make things harder to follow, as there is no documentation what the individual fields mean, and the deep nesting in places where they are being generated makes things even more complex. To address Campbell's concern, maybe the code path that currently returns the 'rename info tuple' should just return a class (instance). And that class (instance) can then have functions for returning the 'data type' and the 'rename info tuple', so that those two things are closely tied. And of course the 'rename info tuple' should (IMO) be replaced by something more self-documenting; at the minimum a `NamedTuple`, but I think a `dataclass` would be more appropriate.

Christoph Lendenfeld commented

2024-03-22 09:47:27 +01:00

Based on the feedback I am closing this PR
It was meant as a simple first step to refactoring that code, but that's not the way forward. Rather we need a general overhaul that changes more things at once

Based on the feedback I am closing this PR It was meant as a simple first step to refactoring that code, but that's not the way forward. Rather we need a general overhaul that changes more things at once

Christoph Lendenfeld closed this pull request

2024-03-22 09:47:30 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Refactor: separate function of WM_OT_batch_rename #119160

Pull request closed