[devtools translations] catch invalid specifiers #13472

pull HashUnlimited wants to merge 2 commits into bitcoin:master from ToDoThings:HashUnlimited-translate-1 changing 1 files +10 −5

HashUnlimited commented at 7:02 pm on June 14, 2018: contributor

While being careful in the source code, during translation a “%” can easily sneak init the wrong position, especially when dealing with percentages (how comes?). Instead of giving up, catching that can make life easier.
[devtools translations] catch invalid specifiers 2f46efa105
lubuzzo approved
in contrib/devtools/update-translations.py:57 in 2f46efa105 outdated
51@@ -52,6 +52,10 @@ def find_format_specifiers(s): 52 percent = s.find('%', pos) 53 if percent < 0: 54 break 55+ try: 56+ specifiers.append(s[percent+1]) 57+ except:
MarcoFalke commented at 8:16 pm on June 14, 2018:

Please no wildcard catches

HashUnlimited commented at 8:56 pm on June 14, 2018:

While sharing the vision of aiming for perfectionism, a proper “validation” of specifiers might be difficult, if going for more than a simple notification then I would also try to cover possible cases where the input data can be misinterpreted by missing a trailing space. I will think about a better while still easy solution and update the PR.

ken2812221 commented at 1:23 am on June 15, 2018:

What exception specifiers.append(s[percent+1]) is going to throw?

HashUnlimited commented at 6:34 am on June 15, 2018:

in certain cases string index out of range for example if we have a % followed by ) - “%x stuff done (overall %p %)”

promag commented at 2:14 pm on June 26, 2018:

IMO we should check those cases instead of catching the exception.

HashUnlimited commented at 2:54 pm on June 26, 2018:

translators accidentally like to sneak in a space after % as well, probably there’s a solution to identify all of those errors. tbh I didn’t think much about it recently

sipa commented at 11:48 pm on June 26, 2018:

I don’t see what other error could be thrown here. Just catch the one you know the code should produce.

HashUnlimited commented at 1:51 pm on June 27, 2018:

you guys are absolutely right, I was thinking about doing some kind of auto-correct but better keep it simple but clean. sorry for not getting to it for a while
meshcollider added the label Scripts and tools on Jun 15, 2018
MarcoFalke deleted a comment on Jun 15, 2018

in contrib/devtools/update-translations.py:59 in 2f46efa105 outdated

51@@ -52,6 +52,10 @@ def find_format_specifiers(s):
52         percent = s.find('%', pos)
53         if percent < 0:
54             break
55+        try:
56+            specifiers.append(s[percent+1])
57+        except:
58+            print('Failed to get specifier')
59         specifiers.append(s[percent+1])

promag commented at 2:13 pm on June 26, 2018:

Should be removed?

catch exact exception ...
... and display the affected string
86807f8b6a
DrahtBot closed this on Aug 25, 2018
DrahtBot reopened this on Aug 25, 2018
DrahtBot closed this on Sep 7, 2018
DrahtBot reopened this on Sep 7, 2018
ken2812221 commented at 2:23 am on September 10, 2018: contributor

Why not just catching this error by linter? The error can be caught whenever someone open a PR.
DrahtBot closed this on May 7, 2019
DrahtBot commented at 5:06 pm on May 7, 2019: member
DrahtBot reopened this on May 7, 2019
laanwj added the label Waiting for author on Jun 17, 2019
laanwj commented at 9:04 am on June 17, 2019: member

Looks like this has many unaddressed comments. Added “Waiting for author” label.
fanquake added the label Up for grabs on Jun 24, 2019
fanquake removed the label Waiting for author on Jun 24, 2019
fanquake commented at 8:05 am on June 24, 2019: member

@laanwj It looks like the author hasn’t been very active on GitHub lately. I’m going to label this “Up for grabs” and close.
fanquake closed this on Jun 24, 2019
MarcoFalke locked this on Dec 16, 2021

Contributors
HashUnlimited lubuzzo MarcoFalke ken2812221 promag sipa DrahtBot laanwj fanquake

Labels
Scripts and tools Up for grabs