search

kohana / minion

Everyone loves having a minion they can boss around
113 stars 76 forks source link

Global fixes: typo, bugs and description #92

Closed WinterSilence closed 10 years ago

kemo commented 10 years ago

I will merge this as soon as these few small finesses are fixed. Good job!

WinterSilence commented 10 years ago

Houston, we have a problem! I sent requests from another computer. how to change these requests?

kemo commented 10 years ago

@WinterSilence which requests, what seems to be the problem?

WinterSilence commented 10 years ago

my local version does not include these changes. how to add them?

WinterSilence commented 10 years ago

understood, i'm stupid :)

kemo commented 10 years ago

I'm not sure you can do that since you removed your github repo. I'll merge this as-is and you can pull from kohana/3.4/develop, fix the remaining things and send another pull request for the fixes.

Nobody is stupid here :)

WinterSilence commented 9 years ago

@cyrgit https://github.com/kohana/minion/blob/3.4/develop/guide/minion/tasks.md

acoulton commented 9 years ago

@WinterSilence that documentation you linked to is inconsistent. The code sample shows $this->options but the text immediately below still refers to taking a $params array.

To be honest, I'm with @cyrgit on this, I don't see any good reason for this breaking change, which is the second I'm aware of (removing ::password being the other) that was introduced in a "typos and bugs" pull request. These really shouldn't have been merged without being split into separate commits with proper messages explaining what was changed, why and the implications.

cyrrill commented 9 years ago

API breaking changes should not be allowed when there is no good reason for it. This just seems like a unilateral decision that was pushed in with the general refactor. Why should I go and update all my Tasks to accommodate for this? Minion was running fine before it. Refactors are good for cleaning up the code, typos, and creating efficiency. If the API is to be changed, it should be a consensus decision, and be made a separate commit from a cleaning refactor.

WinterSilence commented 9 years ago

@acoulton @cyrgit if we return attribute $params is only confuse users because values contained in $this->_options. I will correct the code, if accepted by a few developers.

New version of Kohana will be very different from the previous, so full compatibility is not guaranteed. We must make a choice between backward compatibility and new user-friendly codes.

cyrrill commented 9 years ago

@WinterSilence I don't think it will confuse users to have the $params array in _execute(), as that is the way it has always run. Confusing is to discover your several dozens of Tasks don't run anymore due to a breaking change hidden in a "typos" refactor.

cyrrill commented 9 years ago

On a separate issue from this PR, which shouldn't mix feature requests or breaking changes:

I think the best design for this would actually be to keep a separation between defaults and runtime parameters. Such that $this->options never changes, and whatever is passed in CLI gets sent in through $params.

This allows _execute() to distinguish if the Task is being run with default parameters, or ones sent by the user. When set_options() runs, it overwrites the original defaults the class had defined, making it impossible to know what the default was.

WinterSilence commented 9 years ago

https://github.com/kohana/minion/pull/103

cyrrill commented 9 years ago

@WinterSilence There is already a PR for this pending, no need to duplicate