Refactor: WM_OT_batch_rename #118967

Closed

Christoph Lendenfeld wants to merge 5 commits from ChrisLend/blender:refactor_wm_py into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Christoph Lendenfeld commented

2024-03-01 17:23:04 +01:00

Member

No functional changes.

This PR splits the function _data_from_context on WM_OT_batch_rename

Firstly it removes the parameter check_context. This bool would cause the function
to return something completely different.
Secondly it splits the code from the long if/else chains into separate functions.
Which function is called is determined by a dictionary lookup.

This is in response to the feedback from @dr.sybren on #115185: Fix #115166: Batch Rename Action Clips error

No functional changes. This PR splits the function `_data_from_context` on `WM_OT_batch_rename` Firstly it removes the parameter `check_context`. This bool would cause the function to return something completely different. Secondly it splits the code from the long `if/else` chains into separate functions. Which function is called is determined by a dictionary lookup. ------- This is in response to the feedback from @dr.sybren on [#115185: Fix #115166: Batch Rename Action Clips error](https://projects.blender.org/blender/blender/pulls/115185)

Christoph Lendenfeld added the

Animation & Rigging

labels 2024-03-01 17:23:04 +01:00

Christoph Lendenfeld added 1 commit 2024-03-01 17:23:14 +01:00

9146ed2f69 split code into functions

Christoph Lendenfeld added 1 commit 2024-03-01 17:24:26 +01:00

e3f5523e9a missed two mappings

Sybren A. Stüvel commented

2024-03-05 17:14:40 +01:00

Member

In the current state of the PR there are a lot of calls like this
WM_OT_batch_rename._get_edit_bone_data to access protected static functions.

I am not a big fan of that and usually in such cases I just put the functions outside of the class.
Since I am not sure what the codestyle is for such a case I left them in for now.

I agree that we shouldn't be calling the functions like this.

WM_OT_batch_rename._get_edit_bone_data as called from _data_from_context() can just be refactored to cls._get_edit_bone_data(), as _data_from_context() is a class method. If a @staticmethod needs to call functions from that same class, to me it's an indicator that it's not a static method at all, but should have been a @classmethod.

Depending on how unwieldy things become, I don't mind having these related functions all defined on the same class. It makes it clear tha they belong together.

> In the current state of the PR there are a lot of calls like this > `WM_OT_batch_rename._get_edit_bone_data` to access protected static functions. > > I am not a big fan of that and usually in such cases I just put the functions outside of the class. > Since I am not sure what the codestyle is for such a case I left them in for now. I agree that we shouldn't be calling the functions like this. `WM_OT_batch_rename._get_edit_bone_data` as called from `_data_from_context()` can just be refactored to `cls._get_edit_bone_data()`, as `_data_from_context()` is a class method. If a `@staticmethod` needs to call functions from that same class, to me it's an indicator that it's not a static method at all, but should have been a `@classmethod`. Depending on how unwieldy things become, I don't mind having these related functions all defined on the same class. It makes it clear tha they belong together.

Sybren A. Stüvel reviewed 2024-03-05 17:28:29 +01:00

Sybren A. Stüvel left a comment

Member

Phew, this is quite a beast to untangle! Nice progress, and although I think there's lots else to improve, I think your choice to keep certain bits of code as-is is good. Small steps, makes it easier to figure out where things went wrong if bugs are introduced.

Phew, this is quite a beast to untangle! Nice progress, and although I think there's lots else to improve, I think your choice to keep certain bits of code as-is is good. Small steps, makes it easier to figure out where things went wrong if bugs are introduced.

scripts/startup/bl_operators/wm.py Outdated

						
				@ -2727,1 +2726,3 @@

				    def _data_from_context(cls, context, data_type, only_selected, *, check_context=False):

				    @staticmethod

				    def _data_type_from_context(context):

				        space_type = None if (context.space_data is None) else context.space_data.type

Sybren A. Stüvel commented

2024-03-05 17:15:13 +01:00

Member

This can be simplified to:

space_type = context.space_data and context.space_data.type

or:

space_type = getattr(context.space_data, 'type', None)

but I think the former is a bit nicer.

This can be simplified to: ```py space_type = context.space_data and context.space_data.type ``` or: ```py space_type = getattr(context.space_data, 'type', None) ``` but I think the former is a bit nicer.

scripts/startup/bl_operators/wm.py

						
				@ -2728,0 +2726,4 @@

				    @staticmethod

				    def _data_type_from_context(context):

				        space_type = None if (context.space_data is None) else context.space_data.type

				        space_type_mapping = {

Sybren A. Stüvel commented

2024-03-05 17:25:22 +01:00

Member

These mappings are a bit strange, as their values seem to be keys into space_type_mapping and mode_mapping defined in _data_from_context(). Is this really a necessary indirection? Because if this can be avoided, and these something → data type → function mappings can be reduced to something → function, those functions also don't have to test that they're being called for the right data type any more.

Do you think that would be a possibility?

These mappings are a bit strange, as their values seem to be keys into `space_type_mapping` and `mode_mapping` defined in `_data_from_context()`. Is this really a necessary indirection? Because if this can be avoided, and these `something → data type → function` mappings can be reduced to `something → function`, those functions also don't have to test that they're being called for the right data type any more. Do you think that would be a possibility?

Christoph Lendenfeld commented

2024-03-07 10:22:10 +01:00

Author

Member

I think it would be possible but I'd prefer to do that in a second step. It seems risky to me trying that without understanding how the thing works

I think it would be possible but I'd prefer to do that in a second step. It seems risky to me trying that without understanding how the thing works

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

Christoph Lendenfeld added 2 commits 2024-03-07 10:28:15 +01:00

187400844b Merge branch 'main' into refactor_wm_py

2f4edbba4d call functions with `cls`

Christoph Lendenfeld added 1 commit 2024-03-07 10:49:10 +01:00

0c5ed2c63e use classmethod

Christoph Lendenfeld commented

2024-03-07 11:11:12 +01:00

Author

Member

@dr.sybren with this PR there is an issue where you can only rename Collections in the outliner. I am considering splitting this PR into even smaller steps since I can't tell where that bug is coming from

@dr.sybren with this PR there is an issue where you can only rename Collections in the outliner. I am considering splitting this PR into even smaller steps since I can't tell where that bug is coming from

Sybren A. Stüvel commented

2024-03-07 12:14:21 +01:00

Member

Good idea.

Good idea.

Christoph Lendenfeld referenced this pull request

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

Refactor: separate function of WM_OT_batch_rename #119160

Christoph Lendenfeld commented

2024-04-18 17:23:59 +02:00

Author

Member

going to drop this. This is too hairy code to get into in the near future

going to drop this. This is too hairy code to get into in the near future

Christoph Lendenfeld closed this pull request

2024-04-18 17:24:02 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#118967

No description provided.