typeclassed String to StringLike #82
Conversation
stack.yaml
Outdated
| @@ -4,3 +4,5 @@ packages: | |||
| - scalpel-core/ | |||
| - examples/ | |||
| resolver: lts-13.7 | |||
| extra-deps: | |||
There was a problem hiding this comment.
What happens when scalpel is included as a dependency? Is this extra-dep implicitly carried through? Or will the user need to also explicitly add this dependency?
If they need to explicitly add it, then I think I'd like to wait for LTS to include 0.14.8 before pushing a new release with this dependency.
There was a problem hiding this comment.
It could be pushed to Hackage and then when stackage gets the new version it can be updated
There was a problem hiding this comment.
LTS Haskell 13.21 has 0.14.8 now
| @@ -37,7 +40,7 @@ catComment :: Scraper String String | |||
| catComment = | |||
| -- 1. First narrow the current context to the div containing the comment's | |||
| -- textual content. | |||
| chroot ("div" @: [hasClass "comment", hasClass "text"]) $ do | |||
| chroot (("div" :: TagName String) @: [hasClass "comment", hasClass "text"]) $ do | |||
There was a problem hiding this comment.
I think this is why I chose to mix String and StringLike. It seems a lot less convenient to have to explicitly type each tag name as a TagName String.
Is there a specific benefit that using StringLike enables?
There was a problem hiding this comment.
What if we have a String or Text specific file that just declares the type
There was a problem hiding this comment.
Hrmmm, there's already Text.HTML.Scalpel and Text.HTML.Scalpel.Core. I'd rather not create more re-exports of the API if its not necessary.
Other than API consistency is there a benefit to having StringLike, Text, and String versions of the API? I'm not super familiar with the implementation of OverloadedStrings, but I think the conversion happens at run time. If that's the case I don't think there would be a performance difference since internally, the library is already converting to Text for comparing nodes.
There was a problem hiding this comment.
I am using libs that use Text and it's inconvenient to convert to strings and from strings.
What if we have a classed based file Text.HTML.Scalpel.StringLike and then just apply String to all the functions in Text.HTML.Scalpel?
There was a problem hiding this comment.
Gotcha, I was not really expecting people to be passing dynamic values as attributes. I think having a single Text.HTML.Scalpel.StringLike is a bit more palatable, let's go with that.
| @@ -22,6 +22,7 @@ import Text.HTML.Scalpel.Internal.Select.Types | |||
| import Control.Applicative | |||
| import Control.Monad | |||
| import Data.Maybe | |||
| import Text.StringLike (StringLike, castString) | |||
There was a problem hiding this comment.
Nice! I never really liked that StringLike was a part of the TagSoup package.
There was a problem hiding this comment.
I think StringLike is an abomination as well
scalpel/src/Text/HTML/Scalpel.hs
Outdated
| @@ -157,7 +157,11 @@ module Text.HTML.Scalpel ( | |||
| , seekBack | |||
| , untilNext | |||
| , untilBack | |||
|
|
|||
| -- * Select | |||
| , select | |||
There was a problem hiding this comment.
What is your use case for exposing select?
I'd prefer to keep it hidden since it gives me more flexibility to make changes to the internals of library. Its type has already changed several times over the life of the library.
There was a problem hiding this comment.
You are right. I was thinking I would need it but was able to accomplish what I wanted with functors.
| @@ -37,7 +40,7 @@ catComment :: Scraper String String | |||
| catComment = | |||
| -- 1. First narrow the current context to the div containing the comment's | |||
| -- textual content. | |||
| chroot ("div" @: [hasClass "comment", hasClass "text"]) $ do | |||
| chroot (("div" :: TagName String) @: [hasClass "comment", hasClass "text"]) $ do | |||
There was a problem hiding this comment.
Hrmmm, there's already Text.HTML.Scalpel and Text.HTML.Scalpel.Core. I'd rather not create more re-exports of the API if its not necessary.
Other than API consistency is there a benefit to having StringLike, Text, and String versions of the API? I'm not super familiar with the implementation of OverloadedStrings, but I think the conversion happens at run time. If that's the case I don't think there would be a performance difference since internally, the library is already converting to Text for comparing nodes.
I have replaced the String type in the functions to use StringLike so that Text can easily be used as well as String.