This repository has been archived by the owner on Sep 20, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 51
Fix error handling for non-UTF-8 string in Lexer #93
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<?php | ||
|
||
namespace Hoa\Compiler\Exception; | ||
|
||
use LogicException; | ||
|
||
/** | ||
* It probably points to some internal issue of the Hoa Compiler library. | ||
* Regardless source of the bug, try to report about this exception to the library maintainers. | ||
* Even if bug is yours, this exception must not happen. | ||
*/ | ||
final class InternalError extends LogicException | ||
{ | ||
public function __construct($message, Exception $previous = null) | ||
unkind marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
parent::__construct($message, 0, $previous); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
final
is not required here. I think you can remove it.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.
All exceptions must extend
Hoa\Exception
, see sibling exception in the same directory.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 would also name the class
RegularExpression
orPCRE
, something less generic thanInternalError
.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.
Let's try to see it from another point of view: there is no
final
keyword. There isopen
one. You suggest to open for inheritance. Why? I mean, it's known idea that composition is preferable over inheritance for some objective reasons. So it could be just a language design mistake to make classes open to inheritance by default. Some modern languages, for example, fixed it. So what's the real reason toremoveopen class for inheritance?final
I don't get it either:
RegularExpression
is a special language or pattern, depends on context, but not an error. Kinda weird for exception class name, IMO. You probably rely on namespaces, but II understand and respect your own coding style, but I'm curious how do you explain it.
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 don't use
final
in the existing code base. I prefer to address that as another PR if you don't mind :-). You are pointing to an interesting approach, and I like it, but yeah, keep things seperate a little bit :-).About the class/exception name, maybe
REgularExpression
is not appropriate. But I don't findInternalError
more appropriate, it's too much abstract.The exception represents an error in the lexer. We can (i) reuse the
Hoa\Compiler\Exception\Lexer
exception class (https://github.com/hoaproject/Compiler/blob/master/Exception/Lexer.php), or (ii) create another one to reflect more precisely what is the origin of the exception, hence a better name thanInternalError
.If you decide to re-use
Exception\Lexer
, I'm fine with that. I would even suggest to do that.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.
Hoa\Compiler\Exception\Lexer
doesn't look like internal error of theLexer
. In other words, internal error is library's failure, user did nothing wrong (at least how I interpret it; that's why I reused it forText is not valid utf-8 string, you probably need to switch "lexer.unicode" setting off.
, obvious misconfiguration by user). It's like comparing 4xx HTTP error codes to 5xx.I don't like idea to mix up semantically different errors into one exception.
InternalError
is definitely generic name, I consider this exception as unchecked one; users shouldn't be aware of this exception at all. Thus, they shouldn't catch this type of exception directly (only like\Exception
or\Throwable
in specific parts of application where failure of subprogram can be handled).The thing either works or it is broken. Classifying hypothetical errors is a waste of time for me as most of them are really hypothetical: you don't expect them, otherwise you'd just fixed the; you just make some "save point" for yourself to make sure that it works internally like you expected without surprises. It is similar to
Assertion::blahBlah()
: you don't care about exception class. Assertion just must not fail. If it does, your program is bugged like if you passed object as argument to integer parameter.And regarding names. Practice says that verbs are way more important in programming than nouns. Stack trace says
Lexer
is broken andLexer
throwsLexer
exception. I want to know what happened wrong.LexerEncounteredInvalidUtf8Sequence
,TokenIsEmptyString
, etc.I also was confused when I found class
Lexer
in unit-tests which tried to mimic test of realLexer
. :)But I can reuse
Hoa\Compiler\Exception\Lexer
.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.
Forgot to add: that's why there is no unit test for
InternalError
: it's impossible to achieve via public API of the class unless the code is actually bugged.PhpStorm also doesn't inspect tag
@throws
if exception was inherited from\LogicException
and\RuntimeException
by default, that's why it was inherited from\LogicException
.But in the current moment PhpStorm just goes nuts with exceptions.