Add a parse function to the library #10

Avaq · 2017-05-08T12:31:31Z

I decided to make a new pull-request, as this is a somewhat different solution to the one originally proposed in #8. This keeps the conversation-log clean.

Closes #4
Closes #8 (supersedes)

davidchambers · 2017-05-08T20:46:17Z

index.js

-//. make their data types compatible with the algorithm.
+//. This package exports a function which derives a _type identifier_ from any
+//. JavaScript value, and a [specification][3] for customizing the type
+//. identifier of a value.


This sentence could be improved. As written This package exports applies to both clauses, but the package does not export a specification.

I suggest:

sanctuary-type-identifiers comprises:

an npm and browser -compatible package for deriving the type identifier of a JavaScript value; and

a specification which authors may follow to specify type identifiers for their types.

Avaq · 2017-05-09T09:26:48Z

index.js

+  return {
+    type: type,
+    parse: parse
+  };


Do you agree with the way I exported it? Or would you rather see parse be a property of type, allowing this release to be MINOR.

I prefer type.parse, though not for the reason given. I'd like to avoid type.type. Do you agree that type + type.parse is nicer, or are you opposed to exporting the functions in different ways?

I think the next release should be MAJOR to reflect the change to the specification, even though it is backwards compatible.

davidchambers · 2017-05-09T10:47:56Z

index.js

-//. 6.  Return the [`Object.prototype.toString`][2] representation of `x`
-//.     without the leading `'[object '` and trailing `']'`.
+//.   - the type identifier MUST be a string primitive and SHOULD have format:
+//.     `'<namespace>/<name>[@<version>]'`, where:


This sentence contains two colons. Let's remove the first one.

davidchambers · 2017-05-09T10:50:17Z

index.js

-//. ### Compatibility
+//.       - The `namespace` MUST consist of one or more characters, and SHOULD
+//.         equal the npm package name which defines the type (including
+//.         [scope][4] where appropriate); and


A type is defined by an npm package rather than by an npm package name. Let's replace the npm package name with the name of the npm package.

davidchambers · 2017-05-09T10:53:19Z

index.js

-//.   - every member of the type must have a `constructor` property pointing
-//.     to an object known as the _type representative_;
+//.       - the `version` MUST be a numeric value consisting of one or more
+//.         digits, and it SHOULD represent the version of the type.


Numeric value is redundant. Let's state that the version MUST consist of one or more digits.

davidchambers · 2017-05-09T10:54:16Z

index.js

 //.
-//.   - the type representative's `@@type` property (the _type identifier_)
-//.     must be a string primitive, ideally `'<npm-package-name>/<type-name>'`.
+//.   - If the version is not given, it is assumed to be `0`.


Should this be _version_ to match _name_ and _namespace_?

davidchambers · 2017-05-09T11:21:22Z

index.js

+      parsed[1] == null ? null : parsed[1],
+      parsed[2],
+      parsed[3] == null ? 0 : Number(parsed[3])
+    );


I find TypeIdentifier to be an awkward function. I have thought of a way to remove it which may or may not be sensible. What do you think of the following?

var groups = RPARSE.exec(s); return { namespace: groups == null || groups[1] == null ? null : groups[1], name: groups == null ? s : groups[2], version: groups == null || groups[3] == null ? 0 : Number(groups[3]) };

I like the fact that default values (null and 0) are specified in one place rather than two. Also, the length of the longest line is 79 characters. ;)

⚡ - much nicer this way!

davidchambers · 2017-05-09T11:21:45Z

package.json

@@ -12,6 +12,7 @@
  },
  "dependencies": {},
  "devDependencies": {
+    "doctest": "^0.12.0",


"0.12.x", please. :)

Every time 😒

davidchambers · 2017-05-09T11:27:43Z

test/index.js

+  eq(parse('package/Type@999'), TypeIdentifier('package', 'Type', 999));
+  eq(parse('package/Type@X'), TypeIdentifier('package', 'Type@X', 0));
+  eq(parse('package////@3@2@1@1'), TypeIdentifier('package///', '@3@2@1', 1));
+});


You've done a nice job with these test cases. Each test case exists to test a specific pathway. 👍

davidchambers · 2017-05-09T11:31:30Z

index.js

+  //. 'Null'
+  //.
+  //. > type(true);
+  //. 'Boolean'


I like to omit trailing semicolons when writing doctests. Doctests are meant to resemble REPL interactions, after all. :)

davidchambers · 2017-05-09T11:40:51Z

index.js

+  //. > function Identity(x) { this.value = x; }
+  //. > Identity['@@type'] = 'my-package/Identity@1';
+  //. > type(new Identity(0));
+  //. 'my-package/Identity@1'


The use of new in the usage example bothers me. I'd prefer to make the set-up noisier to avoid it:

//. > function Identity(x) { //. . if (!(this instanceof Identity)) return new Identity(x); //. . this.value = x; //. . } //. . Identity['@@type'] = 'my-package/Identity@1'; //. //. > type(Identity(42)) //. 'my-package/Identity@1'

I didn't know about the multiline syntax either. Nice. :)

davidchambers · 2017-05-09T11:44:38Z

index.js

+  //. {namespace: null, name: 'nonsense!', version: 0}
+  //.
+  //. > function Identity(x) { this.value = x; }
+  //. > Identity['@@type'] = 'my-package/Identity@1';


We could remove these two lines as the definition from the first block of doctests should be in scope. I don't mind the repetition, but I want you to know that the repetition is not required in case you'd prefer to avoid it.

Ah. I was under the impression that each doctest runs in its own scope isolated scope which extends the scope in which the doctest was defined. I'll remove it.

If you're interested in understanding the scoping, run this:

$ node_modules/.bin/doctest --module commonjs --prefix . --print -- index.js

davidchambers · 2017-05-09T11:46:41Z

index.js

+  //# parse :: String -> { namespace :: Nullable String, name :: String, version :: Number }
+  //.
+  //. Takes any String and parses it according to the [specification][3],
+  //. returning an object with a `namespace`, a `name` and a `version` field.


I'd prefer this tweaked version:

- //. returning an object with a `namespace`, a `name` and a `version` field. + //. returning an object with `namespace`, `name`, and `version` fields.

davidchambers · 2017-05-09T11:49:28Z

index.js

+  //.
+  //# type :: Any -> String
+  //.
+  //. Takes any value and produces a String which represents its type. If the


I'd prefer returns in place of produces. Also, I think string could be written in lower case in this context.

And identifies instead of represents?

Identifies sounds good to me, yes. :)

davidchambers · 2017-05-09T11:52:09Z

index.js

+
+  //. ### Usage
+  //.
+  //. `const {parse, type} = require('sanctuary-type-identifiers')`


Could this be in a ```javascript code block?

I think doctest went along and tried to execute it last time I tried, but that may have also had to do with my attempt to put the whole block in a quote-block (>).

Nah, it's fine. It must've been the quote block. ⚡

davidchambers · 2017-05-09T11:55:28Z

index.js

@@ -119,7 +103,46 @@
  //  $$type :: String
  var $$type = '@@type';

-  //  type :: Any -> String
+  //  RPARSE :: RegExp
+  var RPARSE = /^([^]+)\/([^]+?)(?:@(\d+))?$/;


I'm still very much in favour of the version with comments I proposed in #8 (comment).

Avaq · 2017-05-09T15:17:25Z

index.js

+  //. > type.parse(Identity['@@type'])
+  //. {namespace: 'my-package', name: 'Identity', version: 1}
+  //. ```
+  /* eslint-disable key-spacing */


We are breaking the key-spacing rule in most of the parse function.

We want to break the rule, because it looks nice in this case.

Disabling the rule altogether would open the rest of the code up for this mistake.

Putting the comments outside of the function was most aesthetically pleasing.

What do you think?

We want to break the rule, because it looks nice in this case.

I'd go further by claiming that in this case it makes the code easier to comprehend.

Disabling the rule altogether would open the rest of the code up for this mistake.

I like having strict rules as a starting point, but I'm happy to break the rules in some cases. The makefiles for the various Sanctuary projects disable several rules. Let's add --rule 'key-spacing: [off]' to the makefile.

Avaq · 2017-05-09T15:17:43Z

index.js

@@ -129,6 +160,38 @@
      Object.prototype.toString.call(x).slice('[object '.length, -']'.length);
  }

+  //# type.parse :: String -> { namespace :: Nullable String, name :: String, version :: Number }


Would you also have documented this function as type.parse?

Avaq · 2017-05-09T15:32:15Z

index.js

 //.
 //. For example:
 //.
 //. ```javascript
 //. //  Identity :: a -> Identity a
 //. function Identity(x) {
-//.   if (!(this instanceof Identity)) return new Identity(x);
+//.   if (!(this instanceof Identity)) return Identity(x);


davidchambers · 2017-05-09T23:50:27Z

index.js

+  //. ### Usage
+  //.
+  //. ```javascript
+  //. var type = require('sanctuary-type-identifiers')`


s/`/;/

We could use const here rather than var, if you like. :)

davidchambers · 2017-05-09T23:52:53Z

index.js

+  }
+  /* eslint-enable key-spacing */
+
+  type.parse = parse;


I would prefer type.parse = function parse(s) { ... };.

davidchambers · 2017-05-09T23:53:58Z

This is looking very good, Aldwin. I particularly like the way you reorganized the doctests. :)

safareli · 2017-05-10T07:58:34Z

index.js

-//.   - the type representative must have a `@@type` property; and
+//.   - If the type identifier does not conform to the format specified above,
+//.     it is assumed that the entire string represents the _name_ of the type,
+//.     and _namespace_ will be `null`.


I think we should make this change:

- and _namespace_ will be `null` + and _namespace_ and _versoin_ will be `null`

to make it clear that if @@type is T@1 version will not be parsed

+ _versoin_ will be `null`

Version will actually just be 0, as implied by the statement below. Do you think it should be null?

to make it clear that if @@type is T@1 version will not be parsed

That should be conveyed by the entire string represents the name of the type. Do you think it's unclear?

... Do you think it should be null?

My mistake, it should be:

+ and _versoin_ will be `0`

... Do you think it's unclear?

It was a bit unclear for me, as it states "and namespace will be null", but says nothing about version.

Avaq · 2017-05-10T08:27:36Z

index.js

+//.   - If the type identifier does not conform to the format specified above,
+//.     it is assumed that the entire string represents the _name_ of the type.
+//.     _namespace_ will be `null` and _version_ will be `0` in accordance with
+//.     the following statement:


How is this, @safareli? Do you approve of this change @davidchambers?

I like it, though I would drop in accordance with the following statement.

I like it too. +1 for dropping "in accordance with the following statement"

davidchambers · 2017-05-10T12:38:42Z

index.js

-//.   - every member of the type must have a `constructor` property pointing
-//.     to an object known as the _type representative_;
+//.       - the `version` MUST consist of one or more digits, and it SHOULD
+//.         represent the version of the type.


The second clause of each of the sibling points begins and SHOULD rather than and it SHOULD. Let's remove it.

davidchambers · 2017-05-10T12:42:24Z

I have one remaining concern, of a typographical nature. ;)

There are several consistent approaches to formatting lists.

Sentence fragments:

foo
bar
baz

Complete sentence:

foo;
bar; and
baz.

Multiple stand-alone points:

Foo foo foo (foo).
Bar bar. Bar bar.
Baz baz baz.

Currently we have this:

For a type to be compatible with the algorithm:

every member… type representative;

the type representative… (the type identifier);

the type identifier… where:

The namespace… (including scope where appropriate); and

the name… the type; and

the version… the type.

If the type identifier… version will be 0.

If the version is not given, it is assumed to be 0.

We're mixing styles. Also, it seems to me that the last two points don't belong in the list since they are not compatibility requirements.

I suggest this structure:

For a type to be compatible with the algorithm:

every member… type representative;

the type representative… (the type identifier); and

the type identifier… where:

Tthe namespace… (including scope where appropriate); ~~and~~

the name… the type; and

the version… the type.

If the type identifier… version will be 0.

If the version is not given, it is assumed to be 0.

Also, is there a reason to use version in one place and version in others?

Avaq · 2017-05-10T13:35:53Z

is there a reason to use version in one place and version in others?

In the instances where version is used, I refer to the section of the aforementioned string format. In other instances I'm referring to the version in general. I'm not sure if it matters. I see two options:

clarify that we're referring to the section in the string specification by changing it to <version>; or
unify all instances by choosing either version or version.

davidchambers · 2017-05-10T22:26:49Z

In the instances where version is used, I refer to the section of the aforementioned string format.

I thought this might be the case. You don't do things arbitrarily. :)

clarify that we're referring to the section in the string specification by changing it to <version>

I like this option. Additionally, let's drop the definite article: <version> rather than the <version>. Likewise for <namespace> and <name>.

davidchambers · 2017-05-11T10:50:17Z

I pushed one (last!) commit to your branch, Aldwin. Please confirm that you're happy with the changes, then squash your commits. You may also like to view the rendered proof.

Avaq · 2017-05-11T12:19:58Z

Squashed and happy!

This commit adjusts the specification to allow for the inclusion of a version number in the type identifier. It also adds a utility function to the library which takes a string and parses it as a type identifier to separate the different parts. This function is exposed as `type.parse`. Furthermore, because this change asked for a more extensive API documentation, the commit adds validation of the documentation by doctest. Closes #4 Closes #8 (supersedes)

davidchambers · 2017-05-12T03:31:32Z

🎉

Avaq requested review from davidchambers and safareli May 8, 2017 12:31

Avaq force-pushed the avaq/add-type-parser branch from 6fa61d4 to 47ac670 Compare May 8, 2017 12:34

davidchambers reviewed May 8, 2017

View reviewed changes

Avaq commented May 9, 2017

View reviewed changes

davidchambers reviewed May 9, 2017

View reviewed changes

Avaq commented May 9, 2017

View reviewed changes

davidchambers reviewed May 9, 2017

View reviewed changes

safareli reviewed May 10, 2017

View reviewed changes

safareli approved these changes May 10, 2017

View reviewed changes

Avaq commented May 10, 2017

View reviewed changes

davidchambers reviewed May 10, 2017

View reviewed changes

Avaq force-pushed the avaq/add-type-parser branch from 04647c9 to 491fdb2 Compare May 11, 2017 12:18

Avaq force-pushed the avaq/add-type-parser branch from 491fdb2 to 924bf05 Compare May 11, 2017 12:21

Avaq force-pushed the avaq/add-type-parser branch from 924bf05 to a91f7fe Compare May 11, 2017 12:21

davidchambers approved these changes May 12, 2017

View reviewed changes

davidchambers merged commit c9179d9 into master May 12, 2017

davidchambers deleted the avaq/add-type-parser branch May 12, 2017 03:31

Add a parse function to the library #10

Add a parse function to the library #10

Conversation

Avaq commented May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented May 9, 2017

Choose a reason for hiding this comment

Avaq May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented May 10, 2017

Avaq commented May 10, 2017

davidchambers commented May 10, 2017

davidchambers commented May 11, 2017

Avaq commented May 11, 2017

davidchambers commented May 12, 2017

Avaq commented May 8, 2017 •

edited

Loading

Avaq May 9, 2017 •

edited

Loading

Avaq May 9, 2017 •

edited

Loading

Avaq May 10, 2017 •

edited

Loading

Avaq May 10, 2017 •

edited

Loading