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: consumeA<Function|...>(...) should be <Func|...>.consumeFrom(...) #29

Closed
FremyCompany opened this issue Nov 22, 2014 · 2 comments

Comments

@FremyCompany
Copy link
Contributor

I'm willing to make this change in my version of the codebase and make a PR on your project backporting only this change, but only if you accept the idea.

The reason for this is that I don't want to continue to maintain a fork too different from your codebase, and this looks a rather important change; I therefore won't take the rist to make the update on my side if you don't commit on the idea first.

Would you be ok to merge such a pull request if I made one?

@tabatkins
Copy link
Owner

I'm fine with adding this kind of API, but the reason I have the functions named as they are is to just match the spec's algorithm names. There's also some functions that don't map directly to a type (like consumeAListOfDeclarations.

@FremyCompany
Copy link
Contributor Author

Yes, I did see the consume functions for which no type existed, and I was planning to create fake ones for them.

I'm fine with keeping the old function names as aliases, the only intent is to make it easier to understand what's going; for instance it's hard to know right now that the "value" of an AtRule is a SimpleBlock; if the parsing function was closer to the declaration help understand the intent better, I think.

I'll work on that sometime this week, maybe Wednesday.

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

No branches or pull requests

2 participants