-
Notifications
You must be signed in to change notification settings - Fork 36
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
Rewrite for current Syntax Level 3 #13
base: main
Are you sure you want to change the base?
Conversation
Wow, definitely thorough :) So do you think this should just be put alongside the existing scanner package, or should it supersede it? |
tokenizer/token.go
Outdated
} | ||
|
||
// Attempt to turn the token back into a CSS string. | ||
func (t *Token) WriteTo(w io.Writer) { |
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.
This is causing go vet
to raise an error in the Travis tests. The signature should be func (t *Token) WriteTo(w io.Writer) (int64, error)
with the same kind of semantics as the interface (return the number of bytes written and any error).
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.
Yeah, I figured that was going to happen. Do you think I should just do Render() string or implement the byte counting?
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.
At the least it needs to return the error from the underlying writer, so you might as well include the byte counting.
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.
It should be pretty easy if you eliminate the uses of fmt
and just use io.WriteString
everywhere
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.
Ended up using a helper function, called it stickyWriteString(n *int64, err *error, w io.Writer, s string) after the concept of "sticky errors". Not the best name but /shrug
Also updated the README and did another pass over documentation.
|
||
// Handle first character | ||
// dashes allowed at start only for TokenIdent-ish | ||
// eE not allowed at start for Dimension |
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.
What was the reasoning behind this? It causes something like asd {sdf: 3em}
to incorrectly turn into asd {sdf: 3\65 m}
when re-encoding.
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 think this was referenced somewhere as being due to 1e9px
or something like that. Check the spec?
@riking This pull request has some complex conflicts that need to be resolved and then we can review it. Thanks! |
I needed a tokenizer that properly skipped bad-string and bad-url without erroring out, so I just rewrote the whole thing /shrug
Expanded the tests and includes fuzz testing.