-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Feat refactor tasks only using platform library #4665
Conversation
…at-refactor-tasks
…at-refactor-tasks
…at-refactor-tasks
…at-refactor-tasks
…at-refactor-tasks
…rite into feat-refactor-tasks
…at-refactor-tasks
…at-refactor-tasks
…at-refactor-tasks
…at-refactor-tasks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change the hierarchy of files to better support future services and platforms. Right now it seems to focused on the CLI and also create some confusion between CLI and Tasks
src/Appwrite/CLI/TasksService.php
Outdated
{ | ||
$this->type = self::TYPE_CLI; | ||
$this | ||
->addAction(Version::getName(), new Version()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rethinking about this. Do we still need the name if it's part of the interface? Can't we just take it inside the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do that. I think we can change getName to not be a static class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do we do? move getName as instance function? that will have to be done in the Action
and we need the name in order to reference the action, later to remove action?
Not sure I fully understand this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to get them for overwriting them, right?
We could still overwrite them by creating an action with the same name. Do you think it will work?
Co-authored-by: Eldad A. Fux <eldad.fux@gmail.com>
What does this PR do?
Refactors tasks in Appwrite using the new Platform library
Test Plan
Existing tests
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
YES