-
Notifications
You must be signed in to change notification settings - Fork 3
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
add Data.Align.Text #1
base: master
Are you sure you want to change the base?
Conversation
ocramz
commented
Apr 9, 2020
- Add 'text' dependency
- Add 'CHANGELOG.md'
- Moved common types to Data.Align.Types
- Add type signatures in Data.Align.Demo
- Small cosmetic refactorings in Data.Align
- bump package version to 0.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! Minor comments, looks good otherwise.
Optional for bonus points (in separate PR): think up & add some property tests with hedgehog.
|
||
tappend :: (Functor f, Num s) => | ||
f (Trace a s) -> Trace a s -> f (Trace a s) | ||
mt `tappend` (Trace z (t:_)) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move to be a local helper at callsite, as mentioned in #2 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but we're now using this in Data.Align.Text as well. Perhaps an INLINE pragma would work instead?
@@ -0,0 +1,60 @@ | |||
{-# language RecordWildCards #-} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have even anecdotal knowledge, please add a few lines of comment about expectable gains vs using stringy version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think better still would be to set up a benchmark suite.
Now I'm not even too sure about duplicating |
Is the duplication driven by an actual performance need? If not, can as well keep the listy module only, and convert at callsite. Otherwise, doing benchmarks on the real problem (against the simply-wrapping) with Not sure about the typeclass. A bit hackier, but an often employed trick (see Personally I never liked the trick, but the typeclass move is something to be made only if benchmark data verifies its positive effect (passing around dictionaries and whatnot). |