Changed Parsing Method of Identifiers #138
Conversation
|
@ISibboI Sorry. I just checked the test i ran and forgot to run full test. I will look at it. Meanwhile, you can check the logic. |
|
@ISibboI Is there wrong with the new one? |
|
(randomly happened to see this PR - wanted to chime in to say that I've actually benefitted a lot from the flexibility in identifier names, since it lets you define custom conventions that "look like" special syntax but are actually implemented by the Context) |
ISibboI
force-pushed
the
feature_literal_errors
branch
from
53d4507 to
8431650
Compare
There was a problem hiding this comment.
Thanks, this is great.
Due to the comment of @benwr, I would propose to make the identifier parsing generic using a (zero-cost) strategy pattern. This implies quite a large amount of changes though. I would be happy if you would work on that, but otherwise I can also do it.
The Context trait should be extended with an associated type IdentifierValidatorType (or similar). This associated type should be bounded by a trait IdentifierValidator (or similar) with a single function validate_identifier(&str) -> EvalexprResult<()>.
Then, when parsing a literal, it should first attempt to parse boolean and all numeric variants (int, float, scientific notation), and then pass it to validate_identifier. This function can then return any error it wants to if it disagrees with the identifier.
There can be an "any" implementation, that reflects the current behaviour.
An implementation that reflects the Rust behaviour, like what you implemented now.
And also an implementation that allows e.g. only ASCII characters, underscores, numbers and colons, if you want to.
Then this requires a bunch of changes in context implementations that I cannot describe without trying...
So yeah, a bunch of work.
| let contains_alphabet = literal.contains(|x: char| x.is_alphabetic()); | ||
| if let Ok(boolean) = literal.parse::<bool>() { | ||
| Some(Token::Boolean(boolean)) | ||
| } else if starts_with_alphabet_or_underscore |
There was a problem hiding this comment.
The decomposition of the conditions into separate variables is great!
Here we should first check only starts_with_alphabet_or_underscore, and if it does but any of the other three are false, then return a the error variant IllegalIdentifierSequence.
There was a problem hiding this comment.
I did boolean first because true and false would both be interpreted as identifiers.
There was a problem hiding this comment.
I mean that the branch after boolean should only check for starts_with_alphabet_or_underscore, and then should have an inner branch that checks the rest.
| Some(Token::Float(number)) | ||
| } else { | ||
| Some(Token::Identifier(literal.to_string())) | ||
| return Err(EvalexprError::IllegalIdentifierSequence( |
There was a problem hiding this comment.
Here we are parsing the literal as a number, so this should be a new error variant like IllegalNumericLiteral.
There was a problem hiding this comment.
Ok. I will change this
| }, | ||
| _ => Some(Token::Identifier(literal.to_string())), | ||
| _ => { | ||
| return Err(EvalexprError::IllegalIdentifierSequence( |
There was a problem hiding this comment.
Here we are parsing the literal as a number, so this should be a new error variant like IllegalNumericLiteral.
|
Ok. I will probably do this after this weekend. |
ISibboI
force-pushed
the
main
branch
6 times, most recently
from
e4a8571 to
6608b16
Compare
4 participants
No description provided.