Closed sid-kap closed 8 years ago
Hey! Some comments:
i
.OverloadedString
s since there's only one usage of word
, where its type is pinned down by the return type of the parser. A better experiment is someProd <* word' "abc"
, which yields No instance for ListLike...
.word'
?naming: item, token, term
On Sun, Mar 6, 2016 at 8:06 PM, Olle Fredriksson notifications@github.com wrote:
Hey! Some comments:
- The new function should return an i.
- The tests (and examples) aren't a very good benchmark of whether this will work with OverloadedStrings since there's only one usage of word, where its type is pinned down by the return type of the parser. A better experiment is someProd <* word' "abc", which yields No instance for ListLike....
- Can we come up with a better name than word'?
— Reply to this email directly or view it on GitHub https://github.com/ollef/Earley/pull/22#issuecomment-192950397.
(this message was composed with dictation: charitably interpret typos)Sam Boosalis
I would prefer something that either reveals something about the functionality or indicates that it's a version of word
for ListLike
s. It was short-sighted of me to use the contextually rather vague word word
in the first place, but I don't think that's worth changing now. How about just listLike
?
Done on master.
Follow-up to #21.
I'm not sure if it's better to replace
word
altogether with this generalized version, or to simply add a new functionword'
.I believe there wouldn't be problems with replacing
word
withword'
with string literals and OverloadedStrings, because when the compiler seesword' "hello"
, I think it assumes"hello"
is aString
by default, which is a reasonable behavior here. Anyhow, I had no errors compiling the tests after addingOverloadedStrings
and replacingword
withword'
in the tests.(By the way, if we choose to keep both functions, we can simplify the implementation of
word'
to)