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

Do the TODOs + code cleanup. #317

Merged
merged 5 commits into from
Dec 10, 2021
Merged

Do the TODOs + code cleanup. #317

merged 5 commits into from
Dec 10, 2021

Conversation

rahulsprajapati
Copy link
Contributor

@rahulsprajapati rahulsprajapati commented Dec 9, 2021 •
edited
Loading

Description of the Change

Complete Remaining Todos from #260

  • Remove unused garbage collection function. c48e9ae
    • Complete TODO: Classifai/Command/ClassifaiCommand.php: gc
  • Remove unnecessary $service->can_register() check. 1117e23
    • Complete TODO: Classifai/Services/ServicesManager.php: remove this requirement (see: can_register).
  • Remove unused AWS provider. 977c261
    • Complete TODO: Classifai/Providers/AWS/Comprehend.php: Implement reset_settings() method.
    • Complete TODO: Classifai/Providers/AWS/Comprehend.php: Implement can_register() method.
    • Complete TODO: Classifai/Providers/AWS/Comprehend.php: Implement sanitize_settings() method.

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

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

Closes: #260

Changelog Entry

  • Remove unused/unnecessary code:
    • Remove ClassifaiCommand->gc() method.
    • Remove ServicesManager->can_register() method.
    • Remove AWS Provider Comprehend class.

rahulsprajapati and others added 4 commits December 9, 2021 13:36
@rahulsprajapati
Remove unused garbage collection function.
c48e9ae 
Complete TODO: `Classifai/Command/ClassifaiCommand.php:gc`
ref: #260 (comment)
@rahulsprajapati
Remove unnecessary $service->can_register() check.
1117e23 
Complete TODO: Classifai/Services/ServicesManager.php: remove this requirement (see: can_register).
ref: #260 (comment)
@rahulsprajapati
Remove unused AWS provider.
977c261 
ref: #260 (comment)
@jamesmorrison @rahulsprajapati
Switch parent class sanitize_settings to abstract function.
3b6471f
@rahulsprajapati rahulsprajapati changed the title Do the TODOs + cleanup. Do the TODOs + code cleanup. Dec 9, 2021
@rahulsprajapati rahulsprajapati mentioned this pull request Dec 9, 2021
Closed
6 tasks
@rahulsprajapati rahulsprajapati self-assigned this Dec 9, 2021
@jeffpaul jeffpaul requested review from a team and iamdharmesh and removed request for a team December 9, 2021 14:40
@jeffpaul jeffpaul added this to the 1.8.0 milestone Dec 9, 2021
@jeffpaul
Copy link
Member

jeffpaul commented Dec 9, 2021

Adding a note here to credit https://github.com/jamesmorrison && https://github.com/dinhtungdu for their work on #308 which was superseded by this PR.

@jeffpaul jeffpaul merged commit c6c7966 into develop Dec 10, 2021
@jeffpaul jeffpaul deleted the feature/todos-260 branch December 10, 2021 15:19
@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
Reviewers

@iamdharmesh iamdharmesh iamdharmesh approved these changes

Assignees

@rahulsprajapati rahulsprajapati

Labels
None yet
Projects
None yet
Milestone
1.7.1
Development

Successfully merging this pull request may close these issues.

Do the TODOs
4 participants
@rahulsprajapati @jeffpaul @iamdharmesh @jamesmorrison