Add Map module & tests #715

joshburgess · 2019-02-10T00:34:32Z

This PR does the following:

Adds a new Map module for working with ES6 Maps
Adds a getMapSemigroup function to the Semigroup module
Adds a getMapMonoid function to the Monoid module
Adds new tests for all of the above things

I want to help work on adding a String module next. :)

package.json

sledorze

some comments regarding conservative operations in case of a noop

src/Map.ts

gcanti · 2019-02-10T08:24:42Z

@joshburgess thanks for this PR

Not sure I understand what's the problem with jest (--testURL=\"http://localhost\"), I don't get any error from jest with the current configuration.

I propose the following changes:

drop getMapSemigroup in Semigroup
move getMapMonoid from Monoid to Map, needs a Setoid constraint for K
add member function (needs a Setoid constraint), see https://github.com/purescript/purescript-ordered-collections/blob/67a0263f8b1009a26397a811da300f0ef16813c4/src/Data/Map/Internal.purs#L403
the following functions need a Ord constraint for K
- keys
- collect
- toArray
the following functions need a Setoid constraint for K
- insert
- remove
- pop
- isSubmap
- getSetoid
- lookup
- fromFoldable
missing overloadings in toUnfoldable
why the overloadings in insert and remove?
missing overloading in filter
check if unsafeLookup is still useful after all the proposed changes

Also looks like some functions could be optimized using an iterator instead of keys + for ... of.

Example

export const compact = <K, A>(fa: Map<K, Option<A>>): Map<K, A> => {
  const m = new Map<K, A>()
  const entries = fa.entries()
  let e: IteratorResult<[K, Option<A>]>
  while (!(e = entries.next()).done) {
    const [k, oa] = e.value
    if (oa.isSome()) {
      m.set(k, oa.value)
    }
  }
  return m
}

joshburgess · 2019-02-10T10:34:31Z

@joshburgess thanks for this PR

Not sure I understand what's the problem with jest (--testURL=\"http://localhost\"), I don't get any error from jest with the current configuration.

I get 68 errors saying SecurityError: localStorage is not available for opaque origins without that testURL option. Do you have a local jest config file that's in the .gitignore?

gcanti · 2019-02-10T10:41:34Z

@joshburgess no

joshburgess · 2019-02-10T10:49:52Z

@joshburgess no

Hmm, I'm not sure what the problem is. If you google for it, there are many issues about it on GitHub. They all suggest adding this testURL config option in a Jest config file or in the CLI command.

I can't seem to run the tests without it.

jestjs/jest#6769

jestjs/jest#6766

I'll make the other suggested changes tomorrow.

texastoland · 2019-02-10T16:18:45Z

@joshburgess Can you check your jsdom version is 11.10.0 (unaffected) from package-lock.json. jestjs/jest#6792 fixed testURL and released it as [email protected].

joshburgess · 2019-02-11T08:09:51Z

@joshburgess Can you check your jsdom version is 11.10.0 (unaffected) from package-lock.json. facebook/jest#6792 fixed testURL and released it as [email protected].

References to jsdom in the package-lock.json:

"jest-environment-jsdom": "^22.4.1",

"jest-config": {
              "version": "22.4.4",
              "resolved": "https://registry.npmjs.org/jest-config/-/jest-config-22.4.4.tgz",
              ...
              "requires": {
                ...
                "jest-environment-jsdom": "^22.4.1",
                ...
              }
            },

"jest-environment-jsdom": {
      "version": "22.4.3",
      "resolved": "https://registry.npmjs.org/jest-environment-jsdom/-/jest-environment-jsdom-22.4.3.tgz",
      "integrity": "sha512-FviwfR+VyT3Datf13+ULjIMO5CSeajlayhhYQwpzgunswoaLIPutdbrnfUHEMyJCwvqQFaVtTmn9+Y8WCt6n1w==",
      ...
      "requires": {
        ...
        "jsdom": "^11.5.1"
      }
    },

"jsdom": {
      "version": "11.10.0",
      "resolved": "https://registry.npmjs.org/jsdom/-/jsdom-11.10.0.tgz",
      ...
}

gcanti · 2019-02-11T08:14:50Z

@joshburgess have you tried [email protected]? If it solves the problem we could upgrade

joshburgess · 2019-02-11T08:21:05Z

@joshburgess have you tried [email protected]? If it solves the problem we could upgrade

Just tried it. Yes, this fixed it.

joshburgess · 2019-02-11T08:39:43Z

why the overloadings in insert and remove?

I'm not 100% sure, to be honest. I was just following what had already been done for insert and remove in the Record module.

gcanti · 2019-02-11T08:52:36Z

@joshburgess Legacy. They predate the Record module and, as pointed out by @giogonzo, their names are not consistent with the convention used in TypeScript, so I'm going to deprecate them before releasing 1.14.0

joshburgess · 2019-02-11T09:14:49Z

@gcanti Can you explain how I should be using the Setoid<K> for insert and remove? Can I not use the native .has to check for the existence of the key?

gcanti · 2019-02-11T10:12:45Z

@joshburgess native has uses reference equality (===) which makes Map a lot less useful. You can take the Set module as a reference.

So for example let's see lookup (not tested)

/**
 * Lookup the value for a key in a `Map`.
 * If the result is a `Some`, the existing key is also returned.
 */
export function lookupWithKey<K>(S: Setoid<K>): <A>(k: K, m: Map<K, A>) => Option<[K, A]> {
  return <A>(k: K, m: Map<K, A>) => {
    const entries = m.entries()
    let e: IteratorResult<[K, A]>
    while (!(e = entries.next()).done) {
      const [ka, a] = e.value
      if (S.equals(ka, k)) {
        return some(tuple(k, a))
      }
    }
    return none
  }
}

/**
 * Lookup the value for a key in a `Map`.
 */
export function lookup<K>(S: Setoid<K>): <A>(k: K, m: Map<K, A>) => Option<A> {
  const lookupWithKeyS = lookupWithKey(S)
  return (k, m) => lookupWithKeyS(k, m).map(([_, a]) => a)
}

and ~~insert~~ insertAt (for consistency with Array.insertAt) (not tested)

export function insertAt<K>(S: Setoid<K>): <A>(k: K, a: A, m: Map<K, A>) => Map<K, A> {
  const lookupWithKeyS = lookupWithKey(S)
  return (k, a, m) => {
    const found = lookupWithKeyS(k, m)
    if (found.isNone()) {
      const r = new Map(m)
      // add the value
      r.set(k, a)
      return r
    } else if (found.value[1] !== a) {
      const r = new Map(m)
      // overwrite the existing key
      r.set(found.value[0], a)
      return r
    }
    return m
  }
}

@sledorze @giogonzo do you agree?

joshburgess · 2019-02-11T11:45:20Z

@gcanti I think I addressed all of your feedback.

The only thing I didn't change that you suggested was adding overloads to toUnfoldable. I don't see any examples of other modules having overloads for this function. What would they be?

Also, I had to add an Ord constraint for K to toUnfoldable so that I could pass it through to toArray within it.

Also, I removed unsafeLookup.

Could you look over the changes to see whether or not they look good before I go to update the tests?

gcanti · 2019-02-11T12:05:43Z

Could you look over the changes

Sure, I need some time though. In the meanwhile..

adding overloads to toUnfoldable. I don't see any examples

see this PR

gcanti · 2019-02-11T14:12:30Z

@joshburgess

Renaming

rename member to has (since the behavior is aligned with the native method)
rename insertAt to insert (sorry I changed my mind, the naming should be consistent with Record not Array)
rename deleteAt to remove (idem)

Group constraints

toUnfoldable signature: export const toUnfoldable = <F, K>(unfoldable: Unfoldable<F>, O: Ord<K>) => <A>(d: Map<K, A>): HKT<F, [K, A]> => { ... }
getSetoid signature: export const getSetoid = <K, A>(SK: Setoid<K>, SA: Setoid<A>): Setoid<Map<K, A>> => { ... }
getMonoid signature: export const getMonoid = <K, A>(SK: Setoid<K>, SA: Semigroup<A>): Monoid<Map<K, A>> => { ... }

Optimizations

There are many low hanging fruits like this:

Current

export const lookup = <K>(S: Setoid<K>) => <A>(k: K, m: Map<K, A>): Option<A> => {
  const lookupWithKeyS = lookupWithKey(S) // <= created each time
  return lookupWithKeyS(k, m).map(([_, a]) => a)
}

Better

export function lookup<K>(S: Setoid<K>): <A>(k: K, m: Map<K, A>) => Option<A> {
  const lookupWithKeyS = lookupWithKey(S) // <= created once
  return (k, m) => lookupWithKeyS(k, m).map(([_, a]) => a)
}

joshburgess · 2019-02-11T22:40:42Z

rename member to has (since the behavior is aligned with the native method)

This seems weird to me. The Array module doesn't currently do this. It uses member despite the native function being called Array.includes.

I think I would prefer member since this relies on Setoid<K>, unlike the native map.has.

joshburgess · 2019-02-18T08:49:13Z

@joshburgess good idea, we can get rid of sort pretty easily

getting rid of uniq is more annoying but it would be worth it

I made both of these changes. There are no longer any dependencies from Array or Set included in this Map module.

No, how would that work? Let's talk about this in a separate issue / PR though.

Actually, I think I may have misspoke. I'll think about this a bit and get back to you. I'll open an issue and/or PR if I think there's a way.

joshburgess · 2019-02-18T09:15:07Z

@gcanti @texastoland List of new changes other than the above elimination of dependencies:

For the sake of making some progress on undecided things, I renamed has back to member to match the PureScript/Haskell terminology and isMember to elem to match the idea of elem for foldables. I am open to reverting/changing these, but these would be my preferences. I am also open to simply dropping isMember/elem since it is now derivable by getting ahold of the foldable instance via getTraversableWithIndex.
Added elemWithKey. This is essentially the same thing as lookupWithKey, but performs the lookup via the value rather than the key. It returns an Option<[K, A]>.
Added elemKey, based on @texastoland 's suggestion. This uses elemWithKey internally, but maps over its returned Option to transform the pair into just the key, similar to how lookup uses lookupWithKey internally
Refactored isMember (now called elem) to use elemWithKey internally + the .isSome() boolean check, similar to what has (now called member) does with lookup

NOTE: The only odd thing about elemWithKey and elemKey is that they act on only the first occurrence of an equal value. This is normal with lists, but is it weird when iterating over the unordered elements of a map? I think it's okay, but just figured it was worth pointing out.

gcanti · 2019-02-18T10:08:38Z

I renamed has back to member to match the PureScript/Haskell terminology and isMember to elem to match the idea of elem for foldables

👍

@joshburgess to recap

member =        <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => boolean)
lookup =        <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => Option<A>)
lookupWithKey = <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => Option<[K, A]>)

elem =        <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => boolean)
elemKey =     <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<K>)
elemWithKey = <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<[K, A]>)

shouldn't be?

elem =               <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => boolean)
lookupKey (*) =      <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<K>)
lookupKeyWithValue = <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<[K, A]>)

(*) lookupKey is a temporary name

they act on only the first occurrence of an equal value

~~It's not a problem if the map is "valid" with respect to the passed Setoid.~~

EDIT: ^ this is wrong

I think that we should establish once for all whether this module manages only "valid" maps (<= I strongly vote for this) or not.

keysSet and valuesSet can be simplified a lot if we assume the former. Also, if we assume the latter, many signatures, like lookup, don't make much sense.

joshburgess · 2019-02-18T10:08:42Z

Also, just added a new toSet function, similar to keysSet & valuesSet, but returning a Set<K, A>.

joshburgess · 2019-02-18T10:20:22Z

@joshburgess to recap

member =        <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => boolean)
lookup =        <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => Option<A>)
lookupWithKey = <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => Option<[K, A]>)

elem =        <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => boolean)
elemKey =     <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<K>)
elemWithKey = <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<[K, A]>)

shouldn't be?

elem =               <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => boolean)
lookupKey (*) =      <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<K>)
lookupKeyWithValue = <A>(S: Setoid<A>): (<K>(a: A, m: Map<K, A>) => Option<[K, A]>)

(*) lookupKey is a temporary name

Hmm, I'm not sure. I was thinking that the lookup_ variations would always perform the lookup using the key + a Setoid<K>, while the elem_ variations would ignore the key and "lookup" the A value via a Setoid<A>.

I think it's okay that some of them would share the same type signature, because the implementation is different (a lookup based on the key vs a "lookup" based on the value).

In other words, I was tying the name lookup to the behavior of "looking at" the key and the name elem to the behavior of "looking at" the value, not their return types.

It's not a problem if the map is "valid" with respect to the passed Setoid.

I think that we should establish once for all whether this module manages only "valid" maps (<= I strongly vote for this) or not.

keysSet and valuesSet can be simplified a lot if we assume the former. Also, if we assume the latter, many signatures, like lookup, don't make much sense.

Can you further elaborate on this? Sorry, I don't fully understand what you mean by "valid" here.

gcanti · 2019-02-18T16:18:37Z

Constraints and general assumptions

what you mean by "valid" here

By "valid" I mean there's no duplicated keys.

Let's consider lookup:

Current signature:

lookup = <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => Option<A>)

Now if we don't assume that there's no duplicate keys in m with respect to Setoid<K> the signature should be

lookup = <K>(S: Setoid<K>): (<A>(k: K, m: Map<K, A>) => Array<A>)

since it may return 0, 1 or more values for a single key.

EDIT: and since Array<A> should be ordered, the signature becomes

lookup = <K>(O: Ord<K>): (<A>(k: K, m: Map<K, A>) => Array<A>)

I think that the general assumption we should make for this module is the following

maps cannot contain duplicated keys

As a consequence keysSet can be simplified to keysSet = <A>(m: Map<K, A>) => Set<K>.

Drop valuesSet

Thinking twice, I'm not sure this function makes sense: values, as opposed to keys, may be duplicated. Let's drop it.

gcanti · 2019-02-18T16:32:46Z

The only odd thing about elemWithKey and elemKey is that they act on only the first occurrence of an equal value

@joshburgess ah that's right.. both elemKey and elemWithKey don't make much sense for a map. Let's drop them.

EDIT: just noticed a toSet function. Same as above, let's drop it.

joshburgess · 2019-02-19T05:09:33Z

Ok, I deleted those functions & their tests, but the build is failing now for some reason: Error: Cannot find module 'definitelytyped-header-parser'

gcanti · 2019-02-19T10:42:56Z

@joshburgess Thanks!

joshburgess added 6 commits February 9, 2019 19:16

Add Map module, getMapSemigroup, getMapMonoid, & tests

f7ef682

Merge branch 'master' of github.com:gcanti/fp-ts

e3e9b09

Remove accidentally left in comments & re-run test script

17334d2

Replace deepEqual with deepStrictEqual in tests

438ae92

Fix traverseWithKey Map test

eee944e

Add tests for keys and unsafeLookup

1fef2cf

texastoland reviewed Feb 10, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

sledorze reviewed Feb 10, 2019

View reviewed changes

src/Map.ts Outdated Show resolved Hide resolved

src/Map.ts Outdated Show resolved Hide resolved

src/Map.ts Outdated Show resolved Hide resolved

giogonzo reviewed Feb 10, 2019

View reviewed changes

src/Map.ts Outdated Show resolved Hide resolved

sledorze mentioned this pull request Feb 10, 2019

Record: optimise filterWithIndex (conservative) and traverseWithKey (less object creations) (fixes #711) #725

Merged

Remove testURL option & update Jest

6f3971c

joshburgess added 3 commits February 11, 2019 06:35

Address PR feedback -- WIP (need to update tests)

1c159ce

Update fromFoldable to use Setoid<K>

c304e2b

Update collect to use lookupWithKey

0889930

joshburgess added 7 commits February 18, 2019 03:07

Improve keys test + add and test values & valuesSet

b5e9141

Add elemKey and test

af8a2c5

Add elemPair + use it in elemKey & isMember

2f72dd2

Adjust comments

ac87ca9

Rename isMember to elem

137ac2c

Rename has to member

9eef791

Add missing test for elemPair

4faba5b

joshburgess added 2 commits February 18, 2019 03:54

Fix comment for elemPair

8594794

Rename elemPair to elemWithKey

2467bc4

joshburgess added 2 commits February 18, 2019 05:02

Add & test new toSet function

d471a17

Minor cleanup of tests

1764e8d

joshburgess added 2 commits February 18, 2019 23:47

Remove elemKey, elemWithKey, toSet, keysSet, & valuesSet

e3e929e

Merge branch 'master' of github.com:gcanti/fp-ts

22afbb6

joshburgess added 6 commits February 19, 2019 03:25

Update dtslint

2e4a13d

Attempt to fix dtslint package lock problem

204e2ac

Try to fix dtslint issues

152976f

Move dtslint back to devDependencies

5745c3e

Try to fix dtslint yet again

09f0c94

dtslint...

2657989

gcanti merged commit de28de8 into gcanti:master Feb 19, 2019

gcanti added this to the 1.14 milestone Feb 19, 2019

Uh oh!

Add Map module & tests #715

Add Map module & tests #715

Uh oh!

Conversation

joshburgess commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sledorze left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gcanti commented Feb 10, 2019

Uh oh!

joshburgess commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcanti commented Feb 10, 2019

Uh oh!

joshburgess commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

texastoland commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshburgess commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcanti commented Feb 11, 2019

Uh oh!

joshburgess commented Feb 11, 2019

Uh oh!

joshburgess commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcanti commented Feb 11, 2019

Uh oh!

joshburgess commented Feb 11, 2019

Uh oh!

gcanti commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshburgess commented Feb 11, 2019

Uh oh!

gcanti commented Feb 11, 2019

Uh oh!

gcanti commented Feb 11, 2019

Uh oh!

joshburgess commented Feb 11, 2019

Uh oh!

joshburgess commented Feb 18, 2019

Uh oh!

joshburgess commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcanti commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshburgess commented Feb 18, 2019

Uh oh!

joshburgess commented Feb 18, 2019

Uh oh!

gcanti commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcanti commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshburgess commented Feb 19, 2019

Uh oh!

gcanti commented Feb 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

joshburgess commented Feb 10, 2019 •

edited

Loading

joshburgess commented Feb 10, 2019 •

edited

Loading

joshburgess commented Feb 10, 2019 •

edited

Loading

texastoland commented Feb 10, 2019 •

edited

Loading

joshburgess commented Feb 11, 2019 •

edited

Loading

joshburgess commented Feb 11, 2019 •

edited

Loading

gcanti commented Feb 11, 2019 •

edited

Loading

joshburgess commented Feb 18, 2019 •

edited

Loading

gcanti commented Feb 18, 2019 •

edited

Loading

gcanti commented Feb 18, 2019 •

edited

Loading

gcanti commented Feb 18, 2019 •

edited

Loading