-
Notifications
You must be signed in to change notification settings - Fork 43
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
eb772ad
commit 4f56513
Showing
1 changed file
with
10 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4f56513
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 this is slightly easier to read
4f56513
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.
Agreed, though why not use
appTrace
instead ofop Trace
?4f56513
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 guess I feel that we should use our lens accessors whenever we can and that we left in the projections mostly for backwards compatibly. Also using
op <Constructor>
consistently means I don't have to remember newtype record field names. I think this is probably ready to merge now.4f56513
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.
OK, I'm fine with that.
4f56513
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.
We left in some of the projections not just for backwards compatibility, but because they are semantically meaningful operations on their own.
appTrace
means "turn a trace into a function I can apply", which is a meaningful operation on traces. The fact that this so happens to be a newtype projection is just an implementation detail; one can imagine other implementations of trace where we would still have a function calledappTrace
with the same type signature, which is not a simple projection. In my opinion, users should be able to use theTrace
API without actually knowing that it is a newtype.SortedList
, on the other hand, is manifestly a newtype wrapper so using lenses to wrap and unwrap is the API. Anyway, not trying to argue, I don't think it's that important. Was just thinking about this and thought it was worth elaborating a bit.I have a few more changes I want to make and then I'll merge if you're OK with them.