Update regex docs to encourage named regex groups #447 #699
Conversation
Codecov Report
@@ Coverage Diff @@
## master #699 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 24 24
Lines 1522 1522
=====================================
Hits 1522 1522Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Many thanks for raising this!
The groups section is great and is exactly what I was after.
I really appreciate you fixing grammar and adding commas. Could I ask you to have another look over them though as I feel you may have sprinked a few too many around. Specifically see my comments.
If you can just make those tweaks I will get this merged.
docs/matchers/regex.md
Outdated
| @@ -92,11 +92,39 @@ async def remember(opsdroid, config, message): | |||
| await message.respond("OK I'll remember that") | |||
| ``` | |||
|
|
|||
| ### Named Groups | |||
|
|
|||
| Elaborate regular expressions may use many groups, both to capture substrings of interest, as well as to group and structure the regular expression itself. In complex regular expressions, it can become difficult to keep track of the various groups and become unecessarily complex to include multiple groups if you refer to them by their index. Instead, give the groups names to make it easier to keep track of the different groups and retrieve them later. | |||
There was a problem hiding this comment.
The command in ... complex regular expressions, feels weird. I recommend removing it and breaking the sentence into two.
docs/matchers/regex.md
Outdated
|
|
||
| If a skill is configured with both the regex and some other NLU matcher then users who don't use NLU will get a simple regex match. However users with some other NLU configured will get matches on more flexible messages, but will not see duplicate responses where the regex also matched. | ||
| If a skill is configured with both the regex and some other NLU matcher, users who don't use NLU will get a simple regex match. However, users with some other NLU configured will get matches on more flexible messages, but they will not see duplicate responses where the regex also matched. |
There was a problem hiding this comment.
Not 100% sure about how correct the commas are.
removed unnecessary commas in response to feedback given in PR opsdroid#699
|
I made changes based on your feedback and committed them. I see the commit when I click on the Commits tab at the top of this page. Do I need to do more to get the changes reflected in this PR? I'm still pretty new to open source contributing, so I'm not certain that I've done this right. |
|
Nope you've done everything right! Many thanks for this. Also welcome to open source and welcome to opsdroid. Great to have you. |
…oid#699) * Update to encourage named regex groups * Updated Named Groups and commas
Description
Fixes #447
Status
READY |
UNDER DEVELOPMENT|ON HOLDType of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist: