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

Bugfix/91-cssparserhelper-unescape-url #92

Closed

Conversation

shagkur
Copy link
Contributor

@shagkur shagkur commented Jun 7, 2023

This is the PR for issue #91

@phax
Copy link
Owner

phax commented Jun 7, 2023

Haha - also just started an implementation :)

@shagkur
Copy link
Contributor Author

shagkur commented Jun 7, 2023

@phax Tell me if i should Decline this PR?

@phax
Copy link
Owner

phax commented Jun 7, 2023

@shagkur I like my version better because it is a bit "more clear", but I need to crosscheck the consistency to see if it is identical :)

@phax
Copy link
Owner

phax commented Jun 7, 2023

Anyway our effort is highly appreciated!!!

@shagkur
Copy link
Contributor Author

shagkur commented Jun 7, 2023

@phax Yes, maybe there're certain situations where not all escpae sequences are covered in you implementation. The one i've taken (i.e. not written by me) seems to be quite mature on that regard.

@phax
Copy link
Owner

phax commented Jun 7, 2023

@shagkur Yes indeed, your implementation covers more chars.
But e.g. the special character selection does not seem to match the CSS3 syntax:

The release syntax of CSS v3 only uses the hex digits: https://www.w3.org/TR/css-syntax-3/#consume-an-escaped-code-point

Also CSS 2.1 syntax only allows hex-chars but not " !"#$%&\()*+...: https://www.w3.org/TR/CSS2/syndata.html#tokenization

So I would prefer to stick with the "standards based solution". What do you think?

@shagkur
Copy link
Contributor Author

shagkur commented Jun 7, 2023

Indeed all these speical chars handling, as in my implementation, can be omitted and just passed on. I think, from looking at it in more detail, the only special one which needs to be even dropped is the escaped '\n', i.e. "\\n". I believe that's the only one you need to consider in your implementation. Other than that i'm quite fine with your solution and am happy to decline this PR in favour of your fix.

@shagkur
Copy link
Contributor Author

shagkur commented Jun 7, 2023

Btw. will the new released version be available on mavenrepository then?

@shagkur shagkur closed this Jun 7, 2023
@shagkur
Copy link
Contributor Author

shagkur commented Jun 7, 2023

Closed in favour of @phax standard based fix

@phax
Copy link
Owner

phax commented Jun 7, 2023

Okay, noted. Yes of course the new version will be available on Maven Central as 7.0.1 soon

@phax
Copy link
Owner

phax commented Jun 7, 2023

Sorry, no release. See #93 and #91 for the explanation

@shagkur shagkur deleted the bugfix/91-cssparserhelper-unescape-url branch June 8, 2023 06:53
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