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

Expose currently hidden isWhitespace from xml-conduit #149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tolysz
Copy link

@tolysz tolysz commented Jan 29, 2020

This function is useful outside the module itself. It would be beneficial to expose it.

@k0ral
Copy link
Collaborator

k0ral commented Feb 2, 2020

One of the builds fails, for some unrelated reason. I'll have a look at it.

@k0ral
Copy link
Collaborator

k0ral commented Feb 3, 2020

CI build should be fixed now, but your change must be rebased first.

Also, now that I think about it, I am not in favor of exporting isWhitespace as is, for the following reasons:

  • the Text.XML.Stream.Parse module only exports conduits so that you never have to deal with individual Events ; isWhitespace lies in a different, lower level of abstraction, and hardly has a legitimate place in the API of this module;
  • a better place could be in a new Text.XML.Stream.Parse.Internal module, which would export low-level implementation-detail functions (after all, why just isWhitespace ?); but that still looks like a glorified encapsulation breakage;
  • the name isWhitespace sounds like a misnomer, considering its implementation; e.g. does it really make sense for EventBeginDoctype to be called a whitespace ? If we are to export this function, then I would prefer we gave it a better name first.

IMHO, this is one of the very few cases where duplicating the function in your project is a better tradeoff.

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