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

Classifai/Providers/Watson/NLU.php updates #313

Merged
merged 5 commits into from
Nov 5, 2021

Conversation

thrijith
Copy link
Member

Description of the Change

  • Implement can_register() method for Classifai/Providers/Watson/NLU.php.
  • Remove check_license_key from Classifai/Providers/Watson/NLU.php.

Alternate Designs

N/A

Benefits

  • Doesn't load assets unless valid credentials exist.

Possible Drawbacks

N/A

Verification Process

  • Not seeing errors after the change

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#260

Changelog Entry

Implement can_register for NLU.
Remove unused check_license_key method.

@jeffpaul jeffpaul added this to the 1.8.0 milestone Oct 29, 2021
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@thrijith LGTM! Thanks for working on this. Can you please fix the failed workflows before merging this one?

@thrijith
Copy link
Member Author

thrijith commented Nov 3, 2021

Can you please fix the failed workflows before merging this one?

@dinhtungdu it seems the failures were unrelated to the change but I've tried to fix it, please check again when you get a chance, the test failure for one was due to missing function which seems to have been removed in
96fb643#diff-f21acafec2b8e955db72543bf906557e4fa9a409d31026c84953a97c64ae0b93, so I've removed that test, please let me know if that works, thanks!

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 . Great work @thrijith!

@jeffpaul jeffpaul merged commit 7ba4dd1 into develop Nov 5, 2021
@jeffpaul jeffpaul deleted the feature/implement-provider-sanitize branch November 5, 2021 18:03
@jeffpaul jeffpaul linked an issue Dec 9, 2021 that may be closed by this pull request
9 tasks
@jeffpaul jeffpaul modified the milestones: 1.8.0, 1.7.1 Apr 22, 2022
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.

Do the TODOs
3 participants