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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add At/Index class (#43) #48

Merged
merged 2 commits into from
Jun 25, 2018
Merged

Add At/Index class (#43) #48

merged 2 commits into from
Jun 25, 2018

Conversation

leighman
Copy link
Contributor

Thought I'd give this a try - I think I've understood correctly...
Added At for StrMap and Set based on the monocle module. Noticed that PureScript has set always with a Maybe - do we want that here instead?
Is it worth adding atRecord or atOption as PS does?
Wasn't sure about the composition stuff, would appreciate some guidance on that, then could probably look at doing Index too.

src/index.ts Outdated
return new At(at => new Lens(s => SM.lookup(at, s), a => s => a.fold(SM.remove(at, s), x => SM.insert(at, x, s))))
}

export function atSet<A = never>(setoid: Setoid<A>): At<Set<A>, A, boolean> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that S.member(setoid), S.insert(setoid) and S.remove(setoid) can be memoized

export function atSet<A = never>(setoid: Setoid<A>): At<Set<A>, A, boolean> {
  const member = S.member(setoid)
  const insert = S.insert(setoid)
  const remove = S.remove(setoid)
  return new At(
    at => new Lens(s => member(s)(at), a => s => (a ? insert(at, s) : remove(at, s)))
  )
}

@gcanti
Copy link
Owner

gcanti commented Jun 24, 2018

Thanks @leighman

Noticed that PureScript has set always with a Maybe - do we want that here instead?

Since this library is a porting of Scala's monocle I'd stick with monocle definitions

Added At for StrMap and Set based on the monocle module

Let's move them in specific files so that users are not forced to load unwanted dependencies, something along the lines of

- src
  - index.ts (<= At definition)
  - At (<= At instances)
    - StrMap.ts (<= atStrMap instance)
    - Set.ts (<= atSet instance)

Is it worth adding atRecord or atOption as PS does?

  • atOption: no
  • atRecord: not sure what's this, could you please elaborate?

Wasn't sure about the composition stuff

What do you mean, for example fromIso?

@leighman
Copy link
Contributor Author

Like that?

With atRecord I meant an At over a TS Record or object or something for people who are not using StrMap. I guess they could write one if needed.

What do you mean, for example fromIso

Yup, whether it should be asAt in Iso etc.

@gcanti
Copy link
Owner

gcanti commented Jun 24, 2018

I guess they could write one if needed

Yes, if we add a fromIso method to At

export class At<S, I, A> {
  ...
  /** lift an instance of `At` using an `Iso` */
  fromIso<T>(iso: Iso<T, S>): At<T, I, A> {
    return new At(i => iso.composeLens(this.at(i)))
  }
}

then people can build an At for Record<string, A> pretty easily

const isoStrMap = <A = never>() => new Iso<Record<string, A>, SM.StrMap<A>>(s => new SM.StrMap(s), a => a.value)

const atRecord = atStrMap<string>().fromIso(isoStrMap())

@leighman leighman changed the title Add At class (#43) Add At/Index class (#43) Jun 24, 2018
@leighman
Copy link
Contributor Author

Added Index too.
I used strmapIndex and arrayIndex because that's consistent with monocle but it's atStrMap in At. Should we unify them?

@gcanti
Copy link
Owner

gcanti commented Jun 25, 2018

Added Index too

Great, thanks.

Should we unify them?

Yes please, let's rename them to indexStrMap and indexArray.


Are src/At/index.ts, src/Index/index.ts useful? Looks like they are just a burden to maintain

@leighman
Copy link
Contributor Author

Renamed.

Are src/At/index.ts, src/Index/index.ts useful? Looks like they are just a burden to maintain

I agree. Misunderstood your earlier comment. Removed them.

@gcanti gcanti added this to the 1.2 milestone Jun 25, 2018
@gcanti gcanti merged commit c6d88d6 into gcanti:master Jun 25, 2018
@gcanti
Copy link
Owner

gcanti commented Jun 25, 2018

Thanks a lot @leighman

@leighman
Copy link
Contributor Author

Cheers!

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

Successfully merging this pull request may close these issues.

None yet

2 participants