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

detect all regex in sentence #1749

Merged
merged 17 commits into from
Mar 6, 2019
Merged

detect all regex in sentence #1749

merged 17 commits into from
Mar 6, 2019

Conversation

paulaWesselmann
Copy link
Contributor

Proposed changes:

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2019

CLA assistant check
All committers have signed the CLA.

@codeclimate
Copy link

codeclimate bot commented Mar 1, 2019

Code Climate has analyzed commit 9ec7b4b and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Contributor

@MetcalfeTom MetcalfeTom left a comment

Choose a reason for hiding this comment

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

Good work, please add a note in CHANGELOG.rst that you fixed this bug
Other than that, just wanted to re-name a variable so it's a bit clearer what it is supposed to be

Also we can now remove the matches variable entirely.
Implement the above changes and re-tag me for review

rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
MetcalfeTom and others added 3 commits March 4, 2019 15:14
Co-Authored-By: paulaWesselmann <38981219+paulaWesselmann@users.noreply.github.com>
Copy link
Contributor

@MetcalfeTom MetcalfeTom left a comment

Choose a reason for hiding this comment

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

All good!

Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

this needs a couple more style changes

rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
rasa_nlu/featurizers/regex_featurizer.py Outdated Show resolved Hide resolved
@tmbo
Copy link
Member

tmbo commented Mar 6, 2019

ready for another review?

@paulaWesselmann
Copy link
Contributor Author

@tmbo yes please!

Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

looks great 👍

@tmbo tmbo merged commit 04e884f into master Mar 6, 2019
@tmbo tmbo deleted the regex branch March 6, 2019 12:58
@weeebdev
Copy link

weeebdev commented May 10, 2022

@paulaWesselmann @tmbo I think you should make it optional

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re.I in regex match, and possible multiple matches
5 participants