symbols: support group references in replacements by sthibaul · Pull Request #11116 · nvaccess/nvda

sthibaul · 2020-05-05T22:02:11Z

Link to issue number:

Summary of the issue:

Rules such as dates . are quite complex to write: ((?<=\b\d\d)\.(?=\d\d.(\d{2}|\d{4})\b))|((?<=\b\d\d.\d\d)\.(?=(\d{2}|\d{4})\b)) because one wants to change two pieces of text here, and so we have to match them twice. Other rules are even more complex to write (e.g. 1,20€ -> 1 euro 20 needs two complex symbols to perform the two differing ,->euro €->`` replacements.

Description of how this pull request fixes the issue:

This adds support for regexp group references.

Testing performed:

I tested the use of it in the French translation, included in the commit:

dates .        \b(\d\d)\.(\d\d)\.(\d{2}|\d{4})\b
dates /        \b(\d\d)/(\d\d)/(\d{2}|\d{4})\b

dates .        \1 point \2 point \3        all     always  # point de date
dates /        \1 barre oblique \2 barre oblique \3        all     always  # barre oblique de date

The change uses Python's standard support for group, already used for the complex group parsing.

Known issues with pull request:

It requires checking that no symbols file includes a backslash in the replacement string. I did this, and found two (commented) occurrences in the italian symbols file, fixed by the commit. (and incidently found a typo in the hebrew file)

Change log entry:

New features
Symbol pronunciation: Support grouping in complex symbol definition and group references in replacement rule, making them more powerful and simpler. (#11107)

josephsl · 2020-05-05T22:04:51Z

Hi, this is more towards changes for developers, although it would be beneficial to talk about it briefly as part of the user guide if warranted. Thanks.

sthibaul · 2020-05-05T22:08:21Z

Hi, this is more towards changes for developers,

Do you mean that I shouldn't have submitted the pull request here, but rather to another repository?

josephsl · 2020-05-05T22:10:54Z

Hi, no, my comment relates to changes entry. Thanks.

sthibaul · 2020-05-05T22:17:41Z

I added some words in the ./devDocs/developerGuide.t2t

sthibaul · 2020-05-05T22:20:11Z

Hi, no, my comment relates to changes entry.

Ah, ok, I reworded it

sthibaul · 2020-05-05T22:21:19Z

Hi, no, my comment relates to changes entry.

Maybe https://github.com/nvaccess/nvda/wiki/Github-pull-request-template-explanation-and-examples#change-log-entry should say it explicitly that the text should be meant for users?

AAClause · 2020-05-06T12:24:44Z

Hi Samuel :)
It seems to me that modifications made in 'locale/{fr,he,it}/symbols.dic' should be done via Assembla exclusively.
Would you like to join the team of translators?

sthibaul · 2020-05-06T14:05:09Z

Heya André :)
I can drop these from the PR, none of them actually have any effect, it was more for documentation.
I don't really plan to join the translators team, I just meant to pass on at least the date example so people get the idea and can then take benefit from it.

feerrenrut · 2020-05-07T07:31:44Z

@josephsl can you comment on whether it will cause problems for the current translation system if changes are accepted to languages other than English?

josephsl · 2020-05-07T07:33:50Z

Hi, as translations come from SVN and are done by individual translators, it would be best to push to English symbols file, which will then pass onto translators as a diff later. Thanks,

sthibaul · 2020-05-07T07:44:21Z

Here there are no english changes, just fixes in a couple of translations, and an improvement in another one.

sthibaul · 2020-05-07T07:45:57Z

To make things simpler, I have now dropped the locale/ changes, and we can commit the locale/ fixes separately

Adriani90 · 2020-05-10T23:23:48Z

Will this also fix issue #2676?

sthibaul · 2020-05-10T23:29:51Z

Will this also fix issue #2676?

No, it's unrelated, the grouping mentioned here is not of the same kind as in #2676

sthibaul · 2020-05-12T21:58:46Z

So, any comment on including this?

I would like to include the same support in speech-dispatcher which uses the same dict files, but I would rather avoid making the file format diverge...

sthibaul · 2020-05-22T09:13:22Z

Hello, any comment on this?

feerrenrut

Thanks for the contribution @sthibaul and apologies for the delay to review the work.

AppVeyorBot · 2020-06-11T17:58:17Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit c28540f6ab

AppVeyorBot · 2020-06-11T18:11:40Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit d39809229c

AppVeyorBot · 2020-06-11T18:21:39Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit 36cd65d3f6

AppVeyorBot · 2020-06-11T18:41:35Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit 5fdd52145c

…merge Also drop the test for now, will be commited after mergin the translator update.

sthibaul · 2020-08-21T07:53:18Z

Ok, we have sorted out the french translation question. Michel has commited reverting to not using groups, and using norep instead of always.

In this pull request I have dropped the fr_FR test, so that it can be commited without waiting for the french translation update.

When this pull request is merged, Michel will be able to update the french translation to use the group support, and then I'll submit another pull request to add a test for it.

AppVeyorBot · 2020-08-21T07:58:29Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit ddc8590040

CyrilleB79 · 2020-08-21T08:56:20Z

I did not review the code. However I have some remarks:

At first glance, the provided example does not seem realistic: why would a user replace the dot by space when the punctuation level is set to all? I would have expected something to be announced instead at this high level. The replacement rule could be for example (in French):
dates . \1 point \2 point \3 all always # point de date
Maybe I have missed something, so please add an indication to let the reader know the context, i.e. why this rule was added in the French symbol file. @MichelSuch?
Also the semantic of this rule has changed:
- Before the rule was matching a dot in the date and replacing it:
  dates . point all norep # point de date
- Now it matches the whole date and replace it with group substitution:
  dates . \1 \2 \3 all norep # point de date
  Thus I suggest renaming the rule from "dates ." to "date with ." in the example as well as in the translation repo French file when this file will have to be modified (after this PR is merged). Also the associated comment should specify "date avec point" (i.e. "date with dot") instead of "point de date" (i.e. "date dot")
Regarding change log entry something more precise would help such as:
Symbol pronunciation: Support grouping in complex symbol definition and group references in replacement rule

sthibaul · 2020-08-21T10:26:13Z

* why would a user replace the dot by space when the punctuation level is set to all?

Please read the latest version of the pull request, not the initial version. The initial version was using just what was actually in fr/symbols.dic, which happened to be doing exactly that, that's why I had initially just used it as such. That's one of the clarification points we had with Michel. The latest version of the pull request takes as example to replace the dots with "point".

I would have expected something to be announced instead at this high level. The replacement rule could be for example (in French):
dates . \1 point \2 point \3 all always # point de date

That's exactly what is currently in the pull request (but with norep, not always, which would introduce odd effects, that was also part of the clarification points with Michel)

* Also the semantic of this rule has changed:
  
  * Before the rule was matching a dot in the date and replacing it:
    `dates .	 point 	all	norep	# point de date`

No it was not, there was no "point" word there. Against that's part of what Michel fixed.

  * Now it matches the whole date and replace it with group substitution:
    `dates .	\1 \2 \3 	all	norep	# point de date`

Yes, and this is the whole point of this feature: avoid having to use a complex double-case rule, and instead have just one simple rule, with exactly the same effect as before (and the "expected effect" in the french translation was unclear, Michel now cleared up, to appear in git).

    Thus I suggest renaming the rule from "dates ." to "date with ." in the example as well as in the translation repo French file when this file will have to be modified (after this PR is merged).
Also the associated comment should specify "date avec point" (i.e. "date with dot") instead of "point de date" (i.e. "date dot")

As you wish, these parts are up to translator to see what they prefer, I just took what was already there.

sthibaul · 2020-08-21T11:02:01Z

Thus I suggest renaming the rule from "dates ." to "date with ." in the example as well as in the translation repo French file when this file will have to be modified (after this PR is merged).
Also the associated comment should specify "date avec point" (i.e. "date with dot") instead of "point de date" (i.e. "date dot")
As you wish, these parts are up to translator to see what they prefer, I just took what was already there.

Ah, sorry, rereading it I see what you mean: since the effect of the rule is on the whole date and not on the dot, we should rename the name and comment of the rule accordingly. I have now done so.

CyrilleB79 · 2020-08-21T12:27:31Z

Also two other thoughts:

In the symbol and punctuation dialog, the user may edit rules of complex symbols with or without matching groups. However there is nothing to inform the user if the replacement pattern should contain references and how many. The only place where I imagine this information could be written is in the comment of the rule. What do you think?
Maybe the user guide should also be updated in section 12.2.2. The following info may be added:
- in the replacement field, the backslash should be escaped (doubled)
- in the replacement field, \1, \2, etc. should be used to reference 1st, 2nd, etc. matching group
- indicate that the comment should give an indication on the number of the matching groups and on what is expected for the placeholders if required (if this is the accepted solution)

sthibaul · 2020-08-21T12:46:46Z

* In the symbol and punctuation dialog, the user may edit rules of complex symbols with or without matching groups. However there is nothing to inform the user if the replacement pattern should contain references and how many.

I'm not sure to understand: that dialog box contains the Pattern and the Replacement next to each other, is there really a need to make people document how the group replacement works? If they were written separately, I would agree that the interface needs to be documented somewhere, but from what I understand Pattern and Replacement are written together?

* Maybe the user guide should also be updated in section 12.2.2.

Right, I have added some text.

CyrilleB79 · 2020-08-21T15:28:57Z

* In the symbol and punctuation dialog, the user may edit rules of complex symbols with or without matching groups. However there is nothing to inform the user if the replacement pattern should contain references and how many.
I'm not sure to understand: that dialog box contains the Pattern and the Replacement next to each other, is there really a need to make people document how the group replacement works? If they were written separately, I would agree that the interface needs to be documented somewhere, but from what I understand Pattern and Replacement are written together?

The symbol dialog box contains only the rules. For simple symbols, it actually contains the pattern and the replacement next to each other on each line. But for complex symbols, you only have the name of the complex symbol (e.g. . sentence ending) and the replacement (e.g. dot). You do not have the pattern defining the complex symbol. The complex symbol definition can only be defined or modified in the symbols.dic file, not in the dialog box.

* Maybe the user guide should also be updated in section 12.2.2.
Right, I have added some text.

Seems you made the addition in the wrong paragraph, i.e. the paragraph about dictionaries instead of the paragraph about symbol dialog.

sthibaul · 2020-08-21T15:41:29Z

Right, I have added some text.

Seems you made the addition in the wrong paragraph, i.e. the paragraph about dictionaries instead of the paragraph about symbol dialog.

Ah, that's why I was confused in how it would work. The user would indeed need to be notified somehow which group corresponds to what. I have added an initial paragraph, but rule comments would have to specify it indeed

sthibaul · 2020-09-06T22:58:18Z

Any news on this? @feerrenrut perhaps? The french translation question has been sorted out, we will update it after this gets commited.

feerrenrut · 2020-09-10T14:30:10Z

Ok, thanks @sthibaul so I can take it that this is no longer blocked by the French translation and is ready for review and merging?

sthibaul · 2020-09-10T15:00:20Z

Ok, thanks @sthibaul so I can take it that this is no longer blocked by the French translation and is ready for review and merging?

That's it, yes!

sthibaul force-pushed the groups branch from d87b275 to 88e8bca Compare May 5, 2020 22:17

sthibaul force-pushed the groups branch from 88e8bca to e7c3fa0 Compare May 5, 2020 22:53

sthibaul force-pushed the groups branch from e7c3fa0 to 729ac9a Compare May 7, 2020 07:45

feerrenrut suggested changes Jun 11, 2020

View reviewed changes

Comment thread devDocs/developerGuide.t2t Outdated

Comment thread devDocs/developerGuide.t2t Outdated

Comment thread source/characterProcessing.py Outdated

Comment thread source/characterProcessing.py Outdated

sthibaul force-pushed the groups branch from 729ac9a to 3333c60 Compare June 11, 2020 17:10

sthibaul force-pushed the groups branch from 2e45eb3 to e59f56a Compare June 11, 2020 18:02

sthibaul force-pushed the groups branch from e59f56a to a40031b Compare June 11, 2020 18:12

sthibaul force-pushed the groups branch 2 times, most recently from 9b6c2e3 to 0b833e6 Compare June 11, 2020 18:32

sthibaul force-pushed the groups branch from 0b833e6 to 3cb0471 Compare June 11, 2020 18:54

Revert to current fr status, translator has fixed this rule, pending …

56cec12

…merge Also drop the test for now, will be commited after mergin the translator update.

sthibaul added 2 commits August 21, 2020 12:28

Drop unused imports

aa70107

Update dates naming

c16f0f2

CyrilleB79 reviewed Aug 21, 2020

View reviewed changes

Comment thread devDocs/developerGuide.t2t Outdated

Fix text

cbe6832

Add some comments in the user guide as well

6f0a34a

Merge branch 'master' into groups

cb3c057

sthibaul added 2 commits August 21, 2020 17:35

drop bogus paragraph

fae834f

rephrase paragraph for user guide

6de43b9

feerrenrut previously approved these changes Sep 10, 2020

View reviewed changes

feerrenrut removed the blocked label Sep 11, 2020

feerrenrut added 2 commits September 11, 2020 13:42

Merge remote-tracking branch 'origin/master' into groups

4a79307

update changes file for PR nvaccess#11116

e3b3fcc

feerrenrut dismissed their stale review via e3b3fcc September 11, 2020 11:46

feerrenrut approved these changes Sep 11, 2020

View reviewed changes

feerrenrut merged commit d25e3de into nvaccess:master Sep 11, 2020

nvaccessAuto added this to the 2020.4 milestone Sep 11, 2020

sthibaul mentioned this pull request Dec 21, 2020

Symbols: test regex group references through the symbols engine #11961

Merged

Uh oh!

Conversation

sthibaul commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

josephsl commented May 5, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sthibaul commented May 5, 2020

Uh oh!

josephsl commented May 5, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sthibaul commented May 5, 2020

Uh oh!

sthibaul commented May 5, 2020

Uh oh!

sthibaul commented May 5, 2020

Uh oh!

AAClause commented May 6, 2020

Uh oh!

sthibaul commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented May 7, 2020

Uh oh!

josephsl commented May 7, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sthibaul commented May 7, 2020

Uh oh!

sthibaul commented May 7, 2020

Uh oh!

Adriani90 commented May 10, 2020

Uh oh!

sthibaul commented May 10, 2020

Uh oh!

sthibaul commented May 12, 2020

Uh oh!

sthibaul commented May 22, 2020

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Jun 11, 2020

Uh oh!

AppVeyorBot commented Jun 11, 2020

Uh oh!

AppVeyorBot commented Jun 11, 2020

Uh oh!

AppVeyorBot commented Jun 11, 2020

Uh oh!

sthibaul commented Aug 21, 2020

Uh oh!

AppVeyorBot commented Aug 21, 2020

Uh oh!

CyrilleB79 commented Aug 21, 2020

Uh oh!

sthibaul commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sthibaul commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

CyrilleB79 commented Aug 21, 2020

Uh oh!

sthibaul commented Aug 21, 2020

Uh oh!

CyrilleB79 commented Aug 21, 2020

sthibaul commented May 5, 2020 •

edited

Loading

josephsl commented May 5, 2020 via email •

edited by feerrenrut

Loading

josephsl commented May 5, 2020 via email •

edited by feerrenrut

Loading

sthibaul commented May 6, 2020 •

edited

Loading

josephsl commented May 7, 2020 via email •

edited by feerrenrut

Loading

sthibaul commented Aug 21, 2020 •

edited

Loading

sthibaul commented Aug 21, 2020 •

edited

Loading