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.

Sign up for GitHub

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

Jump to bottom

Replace os.path for pathlib in NLU module #7118

Closed
wants to merge 28 commits into from
Closed

Replace os.path for pathlib in NLU module #7118

wants to merge 28 commits into from

Conversation

RomuloSouza
Copy link
Contributor

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @JustinaPetr will take a look at it as soon as possible ✨

@silvasara silvasara requested review from a team and degiz and removed request for a team November 1, 2020 23:54
degiz
degiz suggested changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on that! 🚀
Overall looks very good, just left several comments!

rasa/nlu/classifiers/diet_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/keyword_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/mitie_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/mitie_intent_classifier.py Show resolved Hide resolved
rasa/nlu/classifiers/mitie_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/sklearn_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/featurizers/sparse_featurizer/count_vectors_featurizer.py Outdated Show resolved Hide resolved
rasa/nlu/model.py Outdated Show resolved Hide resolved
rasa/nlu/model.py Outdated Show resolved Hide resolved
rasa/nlu/utils/__init__.py Outdated Show resolved Hide resolved
@degiz
Copy link
Contributor

degiz commented Nov 10, 2020

@silvasara Is the PR ready for the review again? If yes, please re-request the review 👍

@silvasara
Copy link
Contributor

silvasara commented Nov 10, 2020 •
edited
Loading

@silvasara Is the PR ready for the review again? If yes, please re-request the review

Only the author of the PR can request the review again. @RomuloSouza , can you request it again, please?

degiz
degiz reviewed Nov 11, 2020
View reviewed changes
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments 👍
Left a couple more, and we're good to go!

changelog/3153.improvement.md Outdated Show resolved Hide resolved
rasa/nlu/classifiers/mitie_intent_classifier.py Show resolved Hide resolved
rasa/nlu/featurizers/sparse_featurizer/regex_featurizer.py Outdated Show resolved Hide resolved
degiz
degiz approved these changes Nov 11, 2020
View reviewed changes
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot 💯

@RomuloSouza
Copy link
Contributor Author

@degiz are you going to merge this? We'd like to continue the issue for the remaining modules, should we wait for these changes to be merged?

@degiz
Copy link
Contributor

degiz commented Nov 16, 2020

@RomuloSouza I've already approved this patch set, so feel free to update this branch with the latest master and merge it!

@RomuloSouza
Copy link
Contributor Author

@degiz I've updated with the latest master but I still can't merge it. Perhaps because the CI is breaking?

@degiz
Copy link
Contributor

degiz commented Nov 25, 2020

Perhaps because the CI is breaking?

Yes, that's the case. Please check the failing jobs and address the issues 👍

@degiz degiz mentioned this pull request Dec 1, 2020
Closed
4 tasks
@RomuloSouza
Copy link
Contributor Author

@degiz I fixed the issues in the CI and I still can't merge this PR

@degiz
Copy link
Contributor

degiz commented Dec 7, 2020

@RomuloSouza the branch is already behind the master, you need to update it again 😬

Base automatically changed from master to main January 22, 2021 11:15
@tmbo
Copy link
Member

tmbo commented Mar 10, 2021

@RomuloSouza it would be great if you could address the open issues so we don't loose out on this cool contribution - I'll close this as it is stale.

@tmbo tmbo closed this Mar 10, 2021
@ErickGiffoni ErickGiffoni deleted the replace-os.path-pathlib branch August 5, 2021 19:42
@ErickGiffoni ErickGiffoni mentioned this pull request Sep 23, 2021
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silvasara silvasara silvasara left review comments

@degiz degiz degiz approved these changes

@JustinaPetr JustinaPetr Awaiting requested review from JustinaPetr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RomuloSouza @sara-tagger @degiz @silvasara @tmbo