Initial PR #3
Initial PR #3
Conversation
- [X] 100% code coverage - [X] Makefile lint+test+setup - [X] LICENSE MIT - [X] Linting with sanctuary-style - [X] Backwards compatible with paldepind/union-type - [X] Added circle CI yml I'm trying to follow suit with existing projects in the organization. Apologies in advance for any missing pieces.
|
This is very exciting. Thank you, @JAForbes, and thank you @paldepind for creating union-type! |
| jsconfig.json | ||
| .eslintrc | ||
| .vscode | ||
| /coverage/ |
There was a problem hiding this comment.
These comments will reveal my obsessive nature:
- The final line should be terminated by a newline character (
\n). You may like to configure your editor to insert this automatically. - I like to begin all paths with
/so it's clear exactly where we expect a file or directory to reside. (There are situations in which this is not practical.) - I like to end paths to directories with
/. - I like to list directories before files, and order the paths in each of these groups alphabetically.
- Only files created by the project should be listed. On macOS, for example, directories may contain .DS_Store files. One never wants to commit these, so they should be listed in one's global .gitignore. .vscode falls into this category.
- I believe jsconfig.json and .eslintrc could be removed from the list.
There was a problem hiding this comment.
Great points, I'll update my global .gitignore.
| @@ -0,0 +1,25 @@ | |||
| The MIT License (MIT) | |||
|
|
|||
| Copyright (c) 2016 Sanctuary | |||
There was a problem hiding this comment.
Should we include the following as well?
Copyright (c) 2015 Simon Friis Vindum
If you just drew inspiration from union-type this isn't necessary, but if you used its source code as a starting point I believe Simon has copyright. Fortunately union-type is MIT-licensed. :)
/cc @paldepind
There was a problem hiding this comment.
Let's add Copyright for Simon (unless he objects), I didn't reference the source until the final stages but the API design is his.
| WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING | ||
| FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR | ||
| OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
There was a problem hiding this comment.
Let's delete this extra line. :)
| .PHONY: test | ||
| test: | ||
| $(ISTANBUL) cover "test/test.js" -- --recursive | ||
| $(ISTANBUL) check-coverage --branches 98 |
There was a problem hiding this comment.
Let's change the requirement from 98% to 100%. Currently sanctuary-js/sanctuary-set#1 specifies 98 because I integrated Istanbul after the fact and we haven't yet added test cases for the untested paths.
| NPM = npm | ||
|
|
||
| SRC = $(.) | ||
| TEST = $(shell find test -name '*.js' | sort) |
There was a problem hiding this comment.
SRC and TEST aren't referenced in the makefile. Let's remove them.
|
|
||
| .PHONY: lint | ||
| lint: | ||
| $(ESLINT) --env node index.js test/test.js |
There was a problem hiding this comment.
It's not necessary in this case, but I make a habit of including -- to separate keyword arguments from positional arguments:
$(ESLINT) --env node -- index.js test/test.js
|
|
||
| test: | ||
| override: | ||
| - make lint test |
|
|
||
| if (mapped !== t){ | ||
| return mapped; | ||
| } else if (t.constructor === Function) { |
There was a problem hiding this comment.
Let's not rely on object identity for the reasons given in sanctuary-js/sanctuary#100. typeof should be fine here.
| return staticCase(options, this); | ||
| }; | ||
|
|
||
| const Setup = function({check, ENV = T.env}){ |
There was a problem hiding this comment.
I'm opposed to functions with optional arguments (and records with optional fields). I've seen this pattern used in Haskell:
fooWithOpts :: Opts -> Bar -> Baz
foo :: Bar -> Bazfoo is defined simply by applying fooWithOpts to the default options.
We could follow Sanctuary's lead here. Sanctuary provides create, but the exported module is itself the result of applying create to the default options. The only downside of this approach is that the user would need to depend on sanctuary-def in order to disable type checking:
const $ = require('sanctuary-def');
const {create} = require('sanctuary-union-type');
const UnionType = create({check: false, env: $.env});We could, though, export env (as Sanctuary does):
const {create, env} = require('sanctuary-union-type');
const UnionType = create({check: false, env: env});Regardless of what we decide, we should rename the check field to match the name used by sanctuary-def (and Sanctuary).
There was a problem hiding this comment.
I agree with all of this. One note, the naming for check was completely informed by:
https://github.com/paldepind/union-type#disabling-type-checking
But 👍 for having a consistent API within sanctuary libraries.
There was a problem hiding this comment.
Ah, yes, I had wondered whether check came from union-type. Since we'll be providing a different API, we're free to rename the property for consistency. :)
|
|
||
| const Type = T.NullaryType( | ||
| typeName, | ||
| a => a && a['@@type'] === typeName |
There was a problem hiding this comment.
Let's make the guard explicit by replacing a && with a != null &&.
Let's use x as the parameter name to avoid shadowing our type variable named a.
| env: ENV, | ||
| }); | ||
|
|
||
| const CreateUnionType = function(typeName, rawCases, prototype = {}){ |
There was a problem hiding this comment.
Let's avoid this optional parameter. We can ensure that prototype is always provided by making this change:
const Named =
def('UnionType.Named',
{},
[T.String, T.StrMap(T.Any), T.Any],
- CreateUnionType);
+ (x, y) => CreateUnionType(x, y, {}));x and y could be renamed, of course. :)
| Object.keys(rawCases); | ||
|
|
||
| const env = | ||
| ENV.concat(Type); |
There was a problem hiding this comment.
I'd prefer ENV.concat([Type]).
| "main": "index.js", | ||
| "directories": { | ||
| "test": "test" | ||
| }, |
There was a problem hiding this comment.
Is this actually used in any way? The documentation is not particularly enlightening.
There was a problem hiding this comment.
No idea, I think npm init added that automatically.
There was a problem hiding this comment.
Okay. Let's remove it in that case.
| Point.Point(4, 'foo'); | ||
| } catch (e) { | ||
| t.equal(err, e.toString()); | ||
| } |
There was a problem hiding this comment.
I'm not sure about this approach. Won't the assertion only be performed if evaluating the try block results in an exception? I think we need assert.throws here.
We could define a function such as this:
// throws :: (Function, TypeRep a, String) -> Undefined !
const throws = (f, type, message) => {
assert.throws(f, err => err.constructor === type && err.message === message);
};We'd use it like so:
throws(() => { Point.Point(4, 'foo'); },
TypeError,
'...');| 100 | ||
| , area(rect) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Very nice use of block scope! I've never seen this before! 😮
| /* eslint-disable no-var */ | ||
| var List = | ||
| Type({Nil: [], Cons: [T.Any, List]}); | ||
| /* eslint-enable no-var */ |
There was a problem hiding this comment.
This is confusing. This would be much clearer:
const List = Type({Nil: [], Cons: [T.Any, undefined]});I think we can do better though. How about supporting one of the following?
const List = Type({Nil: [], Cons: [T.Any, UnionType.Self]});const List = Type({Nil: [], Cons: [T.Any, UnionType.Recur]});const List = Type({Nil: [], Cons: [T.Any, UnionType.Recursive]});The value of UnionType.Self (or whatever we were to call it) could simply be undefined, but I'd prefer something like {'@@functional/recursive-type-reference': true}.
There was a problem hiding this comment.
Let's discuss this in #4.
There was a problem hiding this comment.
I agree its confusing, using undefined could create recursive types by accident, but I would really like this library to be a superset of paldepind/union-type's API.
I'm torn, on the one hand removing the undefined feature seems like the right API design choice, but then we are no longer supporting the existing API and discussions of API design are suddenly warranted. That isn't a path I want to go down.
But on the other hand, if there was ever a juncture to make a breaking change it would be now.
But ultimately I feel there is more value in continuing the trajectory of keeping this library as a super-set of the original. So I'd prefer to continue to support undefined and in addition support UnionType.self = {'@@functional/recursive-type-reference': true}.
Its a similar decision to supporting paldepind's anonymous union type's in addition to named union-types.
There was a problem hiding this comment.
If we continue to use undefined as the sentinel value users are free to alias it:
const Self = undefined;
// ...
const List = Type({Nil: [], Cons: [a, Self]});I could live with this. :)
sanctuary-def provides a way to define arbitrary types, so we needn't support constructors, predicates, and types.
This requires removing caseOn, which we may decide is not a good idea. If so, we can easily revert this commit.
| @@ -4,6 +4,9 @@ const $ = require('sanctuary-def'); | |||
| const Z = require('sanctuary-type-classes'); | |||
|
|
|||
|
|
|||
| // stripNamespace :: String -> String | |||
| const stripNamespace = s => s.slice(s.indexOf('/') + 1); | |||
There was a problem hiding this comment.
What should happen if input is my-package/sub-package/Point ? maybe we should use lastIndexOf?
There was a problem hiding this comment.
Good question. I've been assuming type identifiers contain one slash (/) at most.
| @@ -176,7 +178,7 @@ test('Destructuring assignment to extract values', () => { | |||
| }); | |||
|
|
|||
| test('Recursive Union Types', () => { | |||
| const List = Type({Nil: [], Cons: [$.Any, undefined]}); | |||
| const List = Type({Nil: [], Cons: [a, undefined]}); | |||
There was a problem hiding this comment.
I think the recursion could be expressed like this:
const List = Type({Nil: [], Cons: (List) => [a, List(a)]});
const Tree = Type({Tip: [a], Node: (Tree) => [Tree(a), Tree(a)]});
|
@JAForbes, shall we close this pull request now that JAForbes/sum-type exists? I will open an initial pull request for a more Sanctuary-style approach when I complete the dozen or so Sanctuary-related tasks currently on my to-do list. |
|
Yes, close away 😊 -----Original Message----- @JAForbes, shall we close this pull request now that JAForbes/sum-type exists? I will open an initial pull request for a more Sanctuary-style approach when I complete the dozen or so Sanctuary-related tasks currently on my to-do list. |
Closes #1
This is a backwards compatible version of @paldepind's brilliant library: union-type.
I haven't started on the readme yet, but I wanted to open the PR as soon as possible so I can get feedback as early as possible.
I've tried to take care of what I could infer first:
I'm trying to follow suit with existing projects in the organisation.
Apologies in advance for any missing pieces.