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

Minor changes which I think makes crass nicer #18

Merged
merged 2 commits into from
May 23, 2024
Merged

Conversation

stoivo
Copy link
Contributor

@stoivo stoivo commented May 22, 2024

For the last couple of days I have been looking at crass and how it parses CSS. I didn't end up making any big changes. I did some changed that I think is nice. I wonder if you are interested in merging any of these changed?

Copy link
Owner

@rgrove rgrove left a comment

Choose a reason for hiding this comment

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

Thanks for the contributions.

I appreciate the spec URL updates in commit ff4b3e4 (minus the changes to test/css-parsing-tests/README.rst, which is in a git subtree and shouldn't be modified) and the test name improvement in commit 08f6145.

However, I'm not interested in the refactoring in commits 91fe420 and 6a71dd7. Those are semver-major changes that would break backwards compatibility, and I don't think they're necessary.

test/css-parsing-tests/README.rst Outdated Show resolved Hide resolved
@stoivo
Copy link
Contributor Author

stoivo commented May 23, 2024

Updated the PR to only two commit you wanted

However, I'm not interested in the refactoring in commits 91fe420 and 6a71dd7. Those are semver-major changes that would break backwards compatibility, and I don't think they're necessary.

If I could make it so it isn't breaking semver would you like to include it then? 6a71dd7 it easy to do without breaking semver. 91fe420 can't be done without breaking semver but I don't think these methods was intendent to be public

Copy link
Owner

@rgrove rgrove left a comment

Choose a reason for hiding this comment

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

Updated the PR to only two commit you wanted

Thank you!

If I could make it so it isn't breaking semver would you like to include it then? 6a71dd7 it easy to do without breaking semver. 91fe420 can't be done without breaking semver but I don't think these methods was intendent to be public

After giving this some thought, I'd prefer not to reorganize things this way, at least for now.

Crass::Parser is intended to be public, even though most users shouldn't need to use it. The methods on Crass are aliases that provide the "simple" API that most users will use, whereas Crass::Parser contains the actual parser implementation and exposes lower level functionality that most users won't need unless they're implementing their own custom parsers.

That said, I would be open to making Crass::stringify an alias for Crass::Parser.stringify, but only an alias — I don't want to move the actual implementation to Crass.

@rgrove rgrove merged commit b51a88e into rgrove:main May 23, 2024
14 checks passed
@stoivo
Copy link
Contributor Author

stoivo commented May 24, 2024

Do you think users are calling instance method consume_component_value and consume_declaration, and this is public api?

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