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

Refactor task plugins #356

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Refactor task plugins #356

merged 1 commit into from
Feb 8, 2024

Conversation

alexb1200
Copy link
Contributor

Refactor the task plugins endpoint to reflect changes to queue and remove wtforms dependency. Resolves #319

Changes

  • remove forms models
  • move errors from controller to service layer
  • move single use function into create method in service layer
  • remove inline type annotations
  • add docstrings to service
  • cleanup docstrings in schema
  • add error if not found flag to all get methods

tests/unit/restapi/task_plugin/test_model.py Outdated Show resolved Hide resolved
tests/unit/restapi/task_plugin/test_service.py Outdated Show resolved Hide resolved
tests/unit/restapi/task_plugin/test_schema.py Outdated Show resolved Hide resolved
tests/unit/restapi/task_plugin/conftest.py Outdated Show resolved Hide resolved
src/dioptra/restapi/task_plugin/controller.py Show resolved Hide resolved
src/dioptra/restapi/task_plugin/controller.py Show resolved Hide resolved
src/dioptra/restapi/task_plugin/controller.py Show resolved Hide resolved
raise TaskPluginDoesNotExistError

return task_plugin
return cast(TaskPlugin, task_plugin)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is needed, service functions already returns TaskPlugin type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this was due to mypy complaining, but if removing the cast still passes all the tests I'm happy to fix it!

@andrewhand
Copy link
Collaborator

365 Reviewed: File changes look clean. All tests passing on local deployment.

@jkglasbrenner
Copy link
Collaborator

jkglasbrenner commented Feb 2, 2024

I've begun my review of this. I also rebased on the latest version of dev and resolved the file conflicts (mostly due to moving all dependency injection config into bootstrap.py in #367). So far, I was also able to consolidate the TaskPluginSchema into a single Schema class.

Will continue working through this later.

@jkglasbrenner jkglasbrenner force-pushed the abyrne-refactor-task-plugins branch from c9dfe6c to d851263 Compare February 2, 2024 23:06
Remove the dependency on WTForms from the taskPlugin endpoint.

Closes #319

Co-authored-by: James K. Glasbrenner <[email protected]>
@jkglasbrenner jkglasbrenner force-pushed the abyrne-refactor-task-plugins branch from d851263 to 2168309 Compare February 8, 2024 22:29
Copy link
Collaborator

@jkglasbrenner jkglasbrenner left a comment

Choose a reason for hiding this comment

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

I've wrapped up with this. The only other change I did was split TaskPluginService into two classes, TaskPluginService and TaskPluginCollectionService so that we only have get() and get_all() methods. I updated the one unit test affected by this change. Otherwise, it's passing all tests and looks good, so I'm going to merge to dev.

@jkglasbrenner jkglasbrenner merged commit 2cadcd5 into dev Feb 8, 2024
19 checks passed
@jkglasbrenner jkglasbrenner deleted the abyrne-refactor-task-plugins branch February 8, 2024 22:34
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.

4 participants