Adding an Error object #148
Conversation
|
Oh.. my normal style of coding is to write a bunch of changes and then see if the CI is green. I guess Github's new "security feature" requires a maintainer's approval to run the CI on this PR. |
lib/ErrorManager.php
Outdated
| if (! $this->configuration->isSilentOnError()) { | ||
| echo '/!\\ ' . $error->__toString() . "\n"; | ||
| } |
There was a problem hiding this comment.
If we want to make an abstraction of the error handling in this library (which I agree with), I would say that we also move this code into an StdErrorReporter or the like (so that we can also e.g. add an GitHubErrorReporter).
There was a problem hiding this comment.
I pushed some changes without the abstraction. When we are adding a third format, then I think it starts to make sense with an abstraction. Right now it is just a simple if statement.
lib/ErrorManager.php
Outdated
| @@ -20,9 +20,48 @@ public function __construct(Configuration $configuration) | |||
| $this->configuration = $configuration; | |||
| } | |||
|
|
|||
| public function addError(Error $error): void | |||
There was a problem hiding this comment.
Might be more handy to have a method that takes scalar arguments and builds an Error VO internally, instead of having addError(new Error(…)) every time
There was a problem hiding this comment.
With this change, I wonder if I should use the existing error/warning method instead. The only downside is the argument order.. But I can live with ->error($message, $throwable, $file, $line)
There was a problem hiding this comment.
Or you could target 0.5.x and break BC :)
There was a problem hiding this comment.
Which way will make it quicker for me to use this feature in my project?
=)
There was a problem hiding this comment.
Well we just released 0.4.0, so it wouldn't make a lot of sense to release 0.5.0 with just that change. That being said, since ErrorManager is not final, adding arguments such as $file and $line would be a BC break for extending classes, so I'm not sure there is a choice either way.
There was a problem hiding this comment.
I just merged #144 that will make 0.5.0 less empty, and also make it less likely for such changes to become BC breaks.
|
Hm.. I need approval again.. |
|
Consider making a PR that upgrades to |
|
I am a doctrine org contributor already... Hm.. anyways: #149 |
|
Could I ask you to do a 0.4.x -> 0.5.x merge? |
Nyholm
force-pushed
the
error
branch
2 times, most recently
from
2373b84
to
7a5b6c6
Compare
|
I've updated the PR to target 0.5.x. CI will be green after 0.4.x -> 0.5.x merge. |
|
I'm AFK until tonight, will try to! |
|
Thank you for the help. Im done with this PR now. |
|
Thanks @Nyholm ! |
|
Wohoo. Thank you for the reviews and merge! |
The end goal of this PR is to be able to run the parser and output a well formatted error message. I would really like to be able to create good "github actions code comments" (or commands). That is why I need to be able to get the errors in a more structured way.
This PR does not break BC.