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

Make TaskNotFoundException more informative by adding task name to exception object #55

Merged
merged 2 commits into from
Oct 7, 2014
Merged

Make TaskNotFoundException more informative by adding task name to exception object #55

merged 2 commits into from
Oct 7, 2014

Conversation

mekras
Copy link
Contributor

@mekras mekras commented Oct 6, 2014

So missing task name can be determined in context where it's not known.

@mekras
Copy link
Contributor Author

mekras commented Oct 6, 2014

The only reason — save task name to have ability to read it later in a catch block. Example case can be found here.

@mekras
Copy link
Contributor Author

mekras commented Oct 6, 2014

By the way, version check can be omitted. "new self" will be enough until TaskNotFoundException has no child classes.

@jaz303
Copy link
Owner

jaz303 commented Oct 6, 2014

I understand the point of the new class and agree that it's a useful change. I'm just unsure why you'd gone for TaskNotFoundException::create() instead of new TaskNotFoundException(), e.g.:

class TaskNotFoundException($taskName) {
    private $taskName;
    public function __construct($taskName) {
        $this->taskName = $taskName;
        parent::__construct();
   }
   public function getTaskName() { return $this->taskName; }
}

throw new TaskNotFoundException($taskName);
?>

@mekras
Copy link
Contributor Author

mekras commented Oct 6, 2014

This will break standard Exception interface in which constructor accepts arbitrary text message. With factory method we preserve default constructor behavior (getTaskName will return empty string in this case) but give a way to set task name and to be sure that it will be exactly task name, not something else.
But that's just my own opinion. It's up to you how to implement this.

@jaz303 jaz303 merged commit 4b540e5 into jaz303:master Oct 7, 2014
@mekras mekras deleted the verbose_notfoundexception branch May 5, 2015 14:32
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.

2 participants