Fix #120140: RNA: Support single quote in path_resolve #120141

YimingWu · 2024-04-01T12:34:37+02:00

YimingWu commented

2024-04-01 12:34:37 +02:00

Support single quote in path_resolve so usages like
bpy.data.path_resolve("objects['Cube']") can work with repr() since
it returns paths with single quoted key.

Support single quote in path_resolve so usages like `bpy.data.path_resolve("objects['Cube']")` can work with `repr()` since it returns paths with single quoted key.

YimingWu added the

Module

Core

label 2024-04-01 12:34:37 +02:00

YimingWu added 1 commit 2024-04-01 12:34:46 +02:00

23688498ca Fix #120140 : RNA: Support single quote in `path_resolve`

Support single quote in path_resolve so usages like
`bpy.data.path_resolve("objects['Cube']")` can work with `repr()` since
it returns paths with single quoted key.

YimingWu added this to the Python API project 2024-04-01 12:34:53 +02:00

YimingWu requested review from Sybren A. Stüvel 2024-04-01 12:35:16 +02:00

YimingWu added 1 commit 2024-04-02 10:07:38 +02:00

b21c5d8184 Add tests to BLI_str_escape_find_quote

Sybren A. Stüvel requested changes 2024-04-02 14:05:34 +02:00

Dismissed

source/blender/blenlib/tests/BLI_string_test.cc

						
				@ -1480,0 +1505,4 @@

				      else {

				        EXPECT_EQ(BLI_str_escape_find_quote_with(item.text, item.quote_character),

				                  item.text + item.expect_offset);

				      }

Sybren A. Stüvel commented

2024-04-02 10:43:13 +02:00

What's the reason to write this like this (with a large percentage of the conditional code duplicated), instead of something like:

const char *expect = item.expect_null ? nullptr : item.text + item.expect_offset;
EXPECT_EQ(BLI_str_escape_find_quote_with(item.text, item.quote_character), expect);

In case of test failure, it would be good to know which exact case failed. Something like this I think would help:

EXPECT_EQ(BLI_str_escape_find_quote_with(item.text, item.quote_character), expect)
  << "text={" << item.text << "}, quote_character=" << item.quote_character
  << ", offset=" << item.expect_offset << ", null=" << item.expect_null;

What's the reason to write this like this (with a large percentage of the conditional code duplicated), instead of something like: ```cpp const char *expect = item.expect_null ? nullptr : item.text + item.expect_offset; EXPECT_EQ(BLI_str_escape_find_quote_with(item.text, item.quote_character), expect); ``` In case of test failure, it would be good to know which exact case failed. Something like this I think would help: ```cpp EXPECT_EQ(BLI_str_escape_find_quote_with(item.text, item.quote_character), expect) << "text={" << item.text << "}, quote_character=" << item.quote_character << ", offset=" << item.expect_offset << ", null=" << item.expect_null; ```

👍 1

source/blender/blenlib/tests/BLI_string_test.cc

						
				@ -1480,0 +1513,4 @@

				TEST_F(StringEscapeFindQuote, Simple)

				{

				  /* NOTE: clang-tidy `modernize-raw-string-literal` is disabled as it causes errors with MSVC.

				   * TODO: investigate resolving with `/Zc:preprocessor` flag. */

Sybren A. Stüvel commented

2024-04-02 10:47:16 +02:00

This comment raises lots of questions. How is this disabled? Because I don't see any change in this PR that disables it. And what errors does it cause? Would this be solved by switching the const char * to a StringRef/StringRefNull/std::string?

This comment raises lots of questions. How is this disabled? Because I don't see any change in this PR that disables it. And what errors does it cause? Would this be solved by switching the `const char *` to a `StringRef`/`StringRefNull`/`std::string`?

YimingWu commented

2024-04-02 15:56:05 +02:00

I have no idea what this part is... I copied something from above 😅... I think it's safe to just remove the comment.

👍 1

source/blender/makesrna/intern/rna_path.cc

						
				@ -143,3 +143,3 @@

				  /* Copy string, taking into account escaped ']' */

				  if (quoted) {

				  if (quote_character) {

				    BLI_str_unescape(buf, *path, len);

Sybren A. Stüvel commented

2024-04-02 11:59:58 +02:00

This code relies on BLI_str_unescape() doing the right thing, but from reading the code of that function I get the feeling that it doesn't take single quotes into account. This will only come into play when a name has both a single quote and a double quote in there, as only having a single quote will not need escaping:

>>> C.object  # named Arm'ature
bpy.data.objects["Arm'ature"]

>>> C.object  # named Arm'atu"re
bpy.data.objects['Arm\'atu"re']

I feel that asking you to change that too is out of scope for this PR (as that would, IMO, also need another unit test, in an area that doesn't have any unit tests at all yet). So just take this as a notice you can scoop aside and ignore at your leasure ;-)

This code relies on `BLI_str_unescape()` doing the right thing, but from reading the code of that function I get the feeling that it doesn't take single quotes into account. This will only come into play when a name has both a single quote and a double quote in there, as only having a single quote will not need escaping: ```python >>> C.object # named Arm'ature bpy.data.objects["Arm'ature"] >>> C.object # named Arm'atu"re bpy.data.objects['Arm\'atu"re'] ``` I feel that asking you to change that too is out of scope for this PR (as that would, IMO, also need another unit test, in an area that doesn't have any unit tests at all yet). So just take this as a notice you can scoop aside and ignore at your leasure ;-)

YimingWu commented

2024-04-02 15:57:38 +02:00

Indeed. This is something I didn't catch, which is expected XD since all the legacy string functions are inter-related. I'll see what I can do with the BLI_str_unescape() part, probably trivial.

Indeed. This is something I didn't catch, which is expected XD since all the legacy string functions are inter-related. I'll see what I can do with the `BLI_str_unescape()` part, probably trivial.

Sybren A. Stüvel commented

2024-04-02 14:06:05 +02:00

Thanks for the PR! I think it's 99% there, just a few questions.

YimingWu added 1 commit 2024-04-03 07:38:56 +02:00

21e2a7ccd5 Support single quote in BLI_str_escape/unescape

Sybren A. Stüvel approved these changes 2024-04-05 11:52:54 +02:00

Sybren A. Stüvel left a comment

LGTM, but since this is so core, I'd love some extra eyes on this. @ideasman42 would you want to review this too?

Campbell Barton requested changes 2024-04-05 13:52:11 +02:00

Campbell Barton left a comment

It's late here, I don't have time for full review but have found problems with this patch, there are places where single quotes isn't implemented it's not clear where they are / aren't supposed to be working. Note that not supporting single quotes was an intentional decision, so this isn't really a bug to fix, more a design decision.

It's late here, I don't have time for full review but have found problems with this patch, there are places where single quotes isn't implemented it's not clear where they are / aren't supposed to be working. Note that *not* supporting single quotes was an intentional decision, so this isn't really a bug to fix, more a design decision.

👀 1

Campbell Barton commented

2024-04-08 07:41:10 +02:00

Replied to #120140 (comment) which notes issues caused by this PR.

Replied to https://projects.blender.org/blender/blender/issues/120140#issuecomment-1162674 which notes issues caused by this PR.

Campbell Barton referenced this pull request

2024-04-08 07:44:52 +02:00

RNA: path_resolve() doesn't deal with single-quoted keys #120140

Merge conflict checking is in progress. Try again in few moments.

View command line instructions.