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

Refactoring, better ability to use cssparser-color as a separate crate. #377

Closed
wants to merge 13 commits into from
Closed

Conversation

chipnertkj
Copy link
Contributor

  • Use case for better cssparser-color separation:
    Access to private implementation items for parsers and other software using the crate.
  • Example use case and motivation:
    My own project. I use the crate to serialize and deserialize CSS3-compatible color data from user input and config files. This was impossible without these changes.
  • Serde impls for color crate items are gated by a new serde feature in the inner crate.
  • A few public functions were adjusted with new names, as suggested by clippy - caused a semantic conflict with std's Iterator.
  • General refactoring.

All checks passed on my fork, but I'm not familiar with the guidelines for commits for this repository. It's my first one here. I hope it's up to expectations. 😁
If something's up, be sure to comment.

@chipnertkj
Copy link
Contributor Author

Apparently part of the changes were also requested here: #376

Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these look good but can we avoid the breaking changes in cssparser? Seem unnecessary to me unless I'm missing something.

@chipnertkj
Copy link
Contributor Author

@emilio That's absolutely doable, it's just that I see valid reasoning in the clippy hint, enough of it to increase the minor version, especially since the project isn't in a mature state yet. Realistically, would that change be problematic? Genuine question.

@emilio
Copy link
Member

emilio commented Apr 5, 2024

Yeah, the tokenizer uses the try prefix (well, now try_parse) for stuff that rewinds, so try_next doesn't feel necessarily like a great name. Also, in a sense the tokenizer / parser's next() is basically an Iterator (over the tokens), even if it returns an extra info for the errors.

It'd also mean we'd have to rewrite quite a few callers. That's not a huge deal if the improvement is clear, but for me at least try_next is just a longer and more confusing name...

In any case, those changes should probably be their own PR. I'd prefer to split the different high-level changes in different PRs if that's possible, so that they can be reviewed / merged independently.

@chipnertkj
Copy link
Contributor Author

I see, sure thing! I'll revert the naming and version changes as soon as possible.

@chipnertkj
Copy link
Contributor Author

Okay, that should be it.

@chipnertkj chipnertkj requested a review from emilio April 6, 2024 08:58
@emilio emilio mentioned this pull request Apr 6, 2024
Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great, but the commit history is a bit of a mess. I sent #378 with it cleaned-up.

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