Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve regex example. #4562

Merged
merged 3 commits into from Sep 6, 2019
Merged

Conversation

btisdall
Copy link
Contributor

@btisdall btisdall commented Aug 9, 2019

  • Remove leading caret since this is likely to lead the casual reader to assume that re.search is being used behind the scenes when it is in fact re.match.
  • Provide a more lucid example of a group match.

@kapilt
Copy link
Collaborator

kapilt commented Aug 9, 2019

Why is re.search match distinction meaningful? Both use the same regex language

The use of Caret here was intentional to have it be a start of string/prefix match as opposed to an embedded body value, as the former is a fairly common use case.

@btisdall
Copy link
Contributor Author

btisdall commented Aug 10, 2019

@kapilt re.match always matches on the start of the string, whereas if the user sees a start anchor in an example they will quite likely assume that it's re.search (supply your own) and then expect this to work:

    - type: value
      op: regex
      key: tag:Squad
      value: foo # why isn't this matching "bar-foo" ?

And that's exactly what I spent an hour on trying to figure out why it didn't work, which ended when I read the source.

@kapilt
Copy link
Collaborator

kapilt commented Aug 22, 2019

thanks, that makes sense and sounds good. sorry about the delay, i'm waiting on confirmation of whether this needs a cla.

if your up for just doing the cla for this contrib, direct link here: https://docs.google.com/forms/d/e/1FAIpQLSfwtl1s6KmpLhCY6CjiY8nFZshDwf_wrmNYx1ahpsNFXXmHKw/viewform

@btisdall
Copy link
Contributor Author

btisdall commented Sep 2, 2019

Hi, sorry for inaction, just got back from vacation. Just submitted the CLA form. Re. the patch, thinking about this some more I think it's better to be explicit about the matching in the docs rather than relying on the user inferring it from the example so I'll adjust it.

* Remove leading caret since this is likely to lead the casual reader to assume that `re.search` is being used behind the scenes when it is in fact `re.match`.
* Provide a more lucid example of a group match.
Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@kapilt kapilt merged commit ed299b5 into cloud-custodian:master Sep 6, 2019
@btisdall btisdall deleted the regex-example-fix branch September 10, 2019 10:09
fidelito pushed a commit to fidelito/cloud-custodian that referenced this pull request May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants