Refactor functors and related packages #131
Conversation
rhendric
force-pushed
the
rhendric/refunctoring
branch
from
fcfe8a7
to
e5674f0
Compare
This is part of a set of commits that rearrange the dependencies between multiple packages. The immediate motivation is to allow certain newtypes to be reused between `profunctor` and `bifunctors`, but this particular approach goes a little beyond that in two ways: first, it attempts to move data types (`either`, `tuple`) toward the bottom of the dependency stack; and second, it tries to ensure no package comes between `functors` and the packages most closely related to it, in order to open the possibility of merging those packages together (which may be desirable if at some point in the future additional newtypes are added which reveal new and exciting constraints on the module dependency graph).
rhendric
force-pushed
the
rhendric/refunctoring
branch
from
e5674f0
to
688543b
Compare
|
Good catch! Mentioned. |
| foldr1 _ (Tuple _ x) = x | ||
| foldl1 _ (Tuple _ x) = x | ||
|
|
||
| instance foldableIdentity :: Foldable1 Identity where |
There was a problem hiding this comment.
Shouldn't this follow naming conventions?
- foldableIdentity
+ foldable1IdentityThere was a problem hiding this comment.
There seems to be a local convention of dropping the 1 in the instance names for the semigroup classes; see lines 88 and 93, for example. I tried to stick to that (but, um, I goofed on line 98)—local consistency trumps global conventions, especially for something as inconsequential as instance names.
There was a problem hiding this comment.
It's certainly possible that lines 88 and 93 are themselves oversights; elsewhere in core libraries I am more familiar with seeing instances match the class. For example, the Foldable1 instances here:
https://github.com/purescript/purescript-nonempty/blob/v5.0.0/src/Data/NonEmpty.purs
I don't have strong opinions on instance names (I wish they were automatic!), but it may be better to fix the instance names on lines 88 and 93 rather than go for local consistency. That said, I would personally accept this PR taking either approach.
| traverse1 f (Tuple x y) = Tuple x <$> f y | ||
| sequence1 (Tuple x y) = Tuple x <$> y | ||
|
|
||
| instance traversableIdentity :: Traversable1 Identity where |
There was a problem hiding this comment.
Same here
- traversableIdentity
+ traversable1Identity| @@ -42,6 +44,14 @@ instance traversableMultiplicative :: Traversable1 Multiplicative where | |||
| traverse1 f (Multiplicative x) = Multiplicative <$> f x | |||
| sequence1 = sequence1Default | |||
|
|
|||
| instance traversableTuple :: Traversable1 (Tuple a) where | |||
There was a problem hiding this comment.
Same here
- traversableTuple
+ traversable1Tuple
rhendric
force-pushed
the
rhendric/refunctoring
branch
from
19deee3
to
be262a0
Compare
|
On second thought, let's get this PR merged. The instance name being renamed won't affect things downstream, but we can at least make it possible for @rhendric to open a PR for |
|
I've opened #132 to track the 'instance name' issue. |
This is part of a set of commits that rearrange the dependencies between
multiple packages. The immediate motivation is to allow certain newtypes
to be reused between
profunctorandbifunctors, but this particularapproach goes a little beyond that in two ways: first, it attempts to
move data types (
either,tuple) toward the bottom of the dependencystack; and second, it tries to ensure no package comes between
functorsand the packages most closely related to it, in order to openthe possibility of merging those packages together (which may be
desirable if at some point in the future additional newtypes are added
which reveal new and exciting constraints on the module dependency
graph).
Description of the change
See discussion in purescript/purescript-profunctor#23.
Checklist: