Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature request] Magma/magma{First,Last} #1114

Closed
ryota-ka opened this issue Feb 1, 2020 · 8 comments
Closed

[feature request] Magma/magma{First,Last} #1114

ryota-ka opened this issue Feb 1, 2020 · 8 comments
Assignees
Milestone

Comments

@ryota-ka
Copy link
Contributor

ryota-ka commented Feb 1, 2020

馃殌 Feature request

For example, Ord exports some basic instances like ordString.
in analogous to that, how about exporting magmaFirst and magmaLast from Magma?

Current Behavior

N/A

Desired Behavior

some basic instances of Magma is exported from fp-ts/lib/Magma.

Suggested Solution

// `src/Magma.ts`

export const magmaFirst = {
  concat<A>(x: A, _: A): A {
    return x;
  },
};

export const magmaLast = {
  concat<A>(_: A, x: A): A {
    return x;
  }
};

Who does this impact? Who is this for?

All users

Describe alternatives you've considered

Not to export such stuff from the module.

Additional context

In Haskell's base package, First and Last are defined as newtypes, and they have corresponding Semigroup instances (Haskell doesn't have Magma in the standard library.)

Your environment

Software Version(s)
fp-ts 2.2.0
TypeScript 3.7.3
@gcanti
Copy link
Owner

gcanti commented Feb 1, 2020

What about using the Semigroup instances?

@ryota-ka
Copy link
Contributor Author

ryota-ka commented Feb 2, 2020

@gcanti oops, I've missed that.
BTW, is there a fair reason not to put them in Magma (which is a more general concept)?

@gcanti
Copy link
Owner

gcanti commented Feb 2, 2020

is there a fair reason not to put them in Magma

Because a Semigroup is also a Magma, so what's the benefit? It would be just a duplication of code

@raveclassic
Copy link
Collaborator

@gcanti We could reuse Magma helpers in Semigroup module:

import { getFirstMagma } from './Magma.ts'
export const getFirstSemigroup = getFirstMagma

What do you think?

@ryota-ka
Copy link
Contributor Author

ryota-ka commented Feb 2, 2020

what's the benefit? It would be just a duplication of code

IMHO we can achieve more proper abstraction of module structure with it.

In my case, Map.fromFoldable requires me to put a Magma instance to it, so I straightforwardly took a look into Magma module, found no pre-defined instance. I thought I have to provide an instance all by myself.
At that moment who will take a look into Semigroup module, which is more specific algebraic structure?
I believe end users will benefit from proper abstraction as they can use the library more intuitively.

Additionally, as @raveclassic points out (thanks!), we can reuse the implementation defined in Magma also in Semigroup, if you wish to avoid a breaking change.

@gcanti
Copy link
Owner

gcanti commented Feb 2, 2020

@raveclassic @ryota-ka are you proposing to move all the semigroup instances to the Magma.ts module?

I'd just add a short comment instead:

/**
 * A `Magma` is a pair `(A, concat)` in which `A` is a non-empty set and `concat` is a binary operation on `A`
 * 
 * See [Semigroup](...) for some instances.
 *
 * @since 2.0.0
 */
export interface Magma<A> {
  readonly concat: (x: A, y: A) => A
}

Magma is (almost) useless, was added to make easier to pass a Semigroup to some APIs (like Map.fromFoldable).

gcanti added a commit that referenced this issue Feb 4, 2020
gcanti added a commit that referenced this issue Feb 4, 2020
@gcanti gcanti added this to the v2.5 milestone Feb 4, 2020
@ryota-ka
Copy link
Contributor Author

ryota-ka commented Feb 7, 2020

@raveclassic @ryota-ka are you proposing to move all the semigroup instances to the Magma.ts module?

At first I didn't mean it, but now I think it's better to have all non-associative instances in Magma.
But it'll be a big change and your comment looks sufficient.
My proposal is not a strong one.

@raveclassic What do you think?

@raveclassic
Copy link
Collaborator

raveclassic commented Feb 9, 2020

I think the comment is fine for now but we should think about moving instances to Magma module once again before v3.0

@gcanti gcanti self-assigned this Feb 11, 2020
gcanti added a commit that referenced this issue Feb 12, 2020
gcanti added a commit that referenced this issue Feb 12, 2020
@gcanti gcanti closed this as completed in caff0d3 Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants