[calsify] AD review of draft-ietf-calext-jscalendar-23

[Barry Leiba <[e-mail]>]

Thanks for the work on this document.  I have a long list of comments,
many of which are just editorial, but a number of which are
substantive.  Because of the length of the list, I’d rather we deal
with all of them before moving the document into last call, so please
digest this list, address what you can directly, and chat with me
about the others.

Throughout: “i.e.” and “e.g.” each needs a comma after it, always.

Throughout: Please use “media type” instead of “MIME type”.

— Abstract —

   It aims to be an
   alternative, and over time successor to, the widely deployed
   iCalendar data format and to be unambiguous, extendable and simple to
   process.


Some misplaced commas here.

NEW
   It aims to be an
   alternative and, over time, a successor to the widely deployed
   iCalendar data format, and to be unambiguous, extendable, and simple
   to process.
END


   In contrast to the JSON-based jCal format, it is not a
   direct mapping from iCalendar and expands semantics where
   appropriate.

I would be clearer about what it *is*, rather than just saying what it
isn’t.  Maybe something like this (tweak accordingly):

NEW
   In contrast to the jCal format, which is also JSON-based, JSCalendar
   is not a direct mapping from iCalendar, but defines the data model
   afresh and expands semantics where appropriate.
END

— Section 1 —

   o  The attributes of the calendar entry represented must be described
      as a simple key-value pair.  Simple events are simple to
      represent, complex events can be modelled accurately.

Number agreement: “as simple key-value pairs” (multiple attributes)
The second sentence is a comma splice; either change the comma to a
semicolon or put “and” after the comma.

   o  The data model should avoid ambiguities and make it difficult to
      make mistakes during implementation.

Is “difficult” really the right word?  Or maybe “less likely”?


      and drop widely unused, obsolete or redundant

“Widely” doesn’t seem apt with a negative such as “unused” (sort of
like how something can be “twice as often used”, but not “twice as
often unused”).  I would say “seldom-used” (and I would add the Oxford
comma, as I did in the Abstract and would do throughout the document,
but I’m not going to push that nor mention it again).

      should be easy with most common iCalendar files but is not
      guaranteed with the full specification.

What does “not guaranteed with the full specification” mean?  can you
maybe rephrase it?

   o  Extensions, such as new properties and components, MUST NOT lead
      to requiring an update to this document.

I think it’s impossible to guarantee that at a “MUST NOT” level.
maybe, “generally do not require updates to this document.”

— Section 1.1 —

   For example, iCalendar defines various formats for local times, UTC
   time and dates, which confuses new users and often leads to
   implementation errors.  Other sources for errors are the requirement
   for custom time zone definitions within a single calendar component,
   as well as the iCalendar format itself; the latter causing
   interoperability issues due to misuse of CR LF terminated strings,
   line continuations and subtle differences between iCalendar parsers.
   The definition of recurrence rules is ambiguous and has resulted in
   differing understandings even between experienced calendar
   developers.

OK, I said I wouldn’t mention it again, but here’s an example of where
something is actually made harder to read by not including the Oxford
comma: I read item 1, “local times”, then item 2, “time and dates”,
then started looking for item 3.  Um.  But in general, I think the
paragraph is clunky and would benefit from being structured as a list.
Does this look reasonable?:

NEW
   Sources of implementation errors include the following:

   - iCalendar defines various formats for local times, UTC time, and dates.

   - iCalendar requires custom time zone definitions within a single
     calendar component.

   - iCalendar’s definition of recurrence rules is ambiguous and has resulted
     in differing understandings even between experienced calendar developers.

   - The iCalendar format itself causes interoperability issues due to misuse
     of CRLF-terminated strings, line continuations, and subtle differences
     among iCalendar parsers.
END

   In recent years, many new products and services have appeared that
   wish to use a JSON representation of calendar data within their API.

Number agreement: “within their APIs.”

   JSCalendar is intended
   to meet this demand for JSON-formatted calendar data, and to provide
   a standard, elegant representation as an alternative to new
   proprietary formats.

The IESG is going to complain about saying what you intend, rather
than what you’re doing, and they will definitely push back on
marketing-sounding things such as claims of elegance.  I also find
“this demand” to lack a clear antecedent.


NEW
   JSCalendar meets the demand for JSON-formatted calendar data that is
   free of such known problems and provides a standard representation
   as an alternative to the proprietary formats.
END

— Section 1.2 —

   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
   document are to be interpreted as described in [RFC2119].

Please use the new BCP 14 boilerplate and add a normative reference to RFC8174.

— Section 1.3 —

   Other types may also be given, with their representation defined
   elsewhere in this document.

Number agreement: “with their representations defined”

— Section 1.4.2 —

   Where "UnsignedInt" is given as a data type, it means an "Int" where
   the value MUST be in the range 0 <= value <= 2^53-1.

Why is there a MUST here, but not in 1.4.1.  I suggest defining these
two (Int and UnsignedInt) consistently.  I don’t much care whether you
use MUST or not, though I would opt for not, so:

NEW
   Where “UnsignedInt" is given as a data type, it means an integer in
   the range 0 <= value <= 2^53-1, represented as a JSON "Number".
END

— Section 1.4.3 —

   In common notation, it should be of the form "YYYY-MM-DDTHH:MM:SSZ".

…which doesn’t allow for fractional seconds, which are allowed if
they’re non-zero.  I’m just noting that; I’m not sure it should be
fixed.

— Section 1.4.4 —

   Instead, they occur in
   every time zone at the same wall-clock time (as opposed to the same
   instant point in time).

I found myself momentarily unsure abnout this sentence.  I suggest a
slight rewording that I think is slightly (just slightly) clearer):

NEW
   Instead, they occur in
   each time zone at the given wall-clock time (as opposed to the same
   instant point in time).

END


— Section 1.4.6 —

       signed-duration = (["+"] / "-") duration

I guess this works.  I think this is a bit cleaner.  No?:

NEW
       signed-duration = ["+" / "-"] duration
END

(I passed the original by a colleague as a check, and he kept looking
at it with varying puzzled expressions before he agreed that he
understood it.  Let’s not make people work that hard.)

— Section 1.4.7 —

   characters from the "URL and Filename Safe" base64 alphabet, as
   defined in Section 5 of [RFC4648]

For absolute, 100% clarity I suggest << "URL and Filename Safe"
base64url alphabet >>.

   Ids need not be unique
   between different objects.

“among”, rather than “between” (the latter implies only two).

   For example, two JSEvent objects MAY use
   the same ids in their respective "links" properties.

While the BCP 14 “MAY” here is arguably not wrong, I think the intent
here isn’t to say what choice is allowed, but to alert the implementer
about what could happen.  So I’d make it “may” (or “might”).

   This does not imply any semantic connection
   between the two.

You give two scenarios with two instances in each, so “between the
two” seems odd.  I would simply say, “These situations do not imply
any semantic connections among the objects.”

   The keys are a path in a subset of
   [RFC6901] JSON pointer format, with an implicit leading "/" (i.e.
   prefix each key with "/" before applying the JSON pointer evaluation
   algorithm).

“The keys are a path”?  There’s at least a number problem here, but I
find the whole sentence to be hard to follow.  Do you mean this?:

NEW
   Each key is a path represented in a subset of
   JSON pointer format [RFC6901].  The paths have an implicit leading "/",
   so each key is prefixed with "/" before applying the JSON pointer
   evaluation algorithm.
END

   1.  The pointer MUST NOT reference inside an array (i.e. it MUST NOT
       insert/delete from an array; the array MUST be replaced in its
       entirety instead).

I don’t see any value in “MUST NOT reference inside an array”, and
there’s a reason that you then clarify it.  Just let the clearer
version stand alone:

NEW
   1.  The pointer MUST NOT
       insert/delete values from an array; the array MUST be replaced in
       its entirety instead.
END

   3.  There MUST NOT be two patches in the PatchObject where the
       pointer of one is the prefix of the pointer of the other, e.g.
       "alerts/foo/offset" and "alerts".

Is “the” correct in “the prefix”, or do you mean “a prefix”?  Are
"alerts/foo/offset" and "alerts/foo" allowed together?

   o  null: Remove the property from the patched object.  If not present
      in the parent, this a no-op.

What’s the subject of the second sentence?  Is it the pointer, the
property, the patched object?  It’s unclear; please say it explicitly,
“If <what?> is not present”.

   o  Anything else: The value to set for this property (this may be a
      replacement or addition to the object being patched).

The structure of this bullet should be parallel to that of the first
bullet: as the first bullet is an instruction, so should this be:

NEW
   o  Anything else: Set the value for the property to this value (this
      may be a replacement or addition to the object being patched).
END

   Implementations MUST reject a PatchObject if any of its patches are
   invalid.

It’s worth being explicit here:

NEW
   Implementations MUST reject in its entirety a PatchObject if any of
   its patches is invalid.  Implementations MUST NOT apply partial patches.
END

— Section 1.4.9 —

   By default, time zones in JSCalendar are identified by their name in
   the IANA Time Zone Database [TZDB], and the zone rules of the
   respective zone record apply.

Number agreement: “by their names” and “zone records”

   Implementations MAY embed the definition of custom time zones in the
   "timeZones" property (see Section 4.7.2).

Number agreement: “definitions”

— Section 1.4.10 —

   The object that defines this
   relation is the linking object, the other object is the linked
   object.

This is a comma splice.  Either change the comma to a semicolon or put
“while” after it (or “and”, but I prefer “while” here).

      Keys in the set MUST be one of the following values, or specified
      in the property definition where the Relation object is used, or
      an IANA-registered value, or a vendor-specific value:

I suggest naming the IANA registry from which the key name has to
come.  Same comment in other places in the document where you say
“IANA-registered value”.

      *  "child": The linked object is a subpart of the linking object.

      *  "parent": The linking object is part of the overall linked
         object.

I don’t understand the definition of “parent”.  Wouldn’t a proper
definition be parallel to the definition of “child”, like this?:

NEW
      *  "parent": The linking object is a subpart of the linkied object.
END

   Note, the Relation object only has one property (except @type); it is
   specified as an object with a single property to allow for extension
   in the future.

Do you mean, “specified as an array with a single property”, or some
such?  Or maybe this?:

NEW
   Note: the “relation” array only contains one property; it is specified
   as an array to allow for extension in the future.
END

— Section 2.2 —

   A JSTask may start and be due at certain points in time, may take
   some estimated time to complete and may recur; none of which is
   required.  This notably differs from JSEvent (Section 2.1) which is
   required to start at a certain point in time and typically takes some
   non-zero duration to complete.

1. The semicolon is incorrect, and should be a comma.

2. In the second sentence, “which is” needs a comma before it.

3. Why is that description of JSEvent here, and not in Section 2.1?

— Section 3.1 —

      types, but normalization of these types is inherently protocol and
      scheme-specific, depending on the use-case

This needs a hyphen on “protocol-“ (it’s short for “protocol-specific”).

   calendar exchange protocol or defined by another RFC.

I would say “defined elsewhere”, or “defined in another document”: it
might not be in an RFC.

— Section 3.2 —

   Some JSCalendar properties allow vendor-specific value extensions.
   If so, vendor-specific values MUST be prefixed with a domain name
   controlled by the vendor, e.g. "example.com/customrel".

There’s no “if so” about it: it’s a fact.  Maybe you want to say,
“Such vendor-specific values MUST…”

— Section 4.1.2 —

   [RFC4122] describes a range of
   established algorithms to generate universally unique identifiers
   (UUID), and the random or pseudo-random version is recommended.

I suggest using more specific reference to 4122, and recommending with
the BCP 14 key word:

NEW
   [RFC4122] describes a range of
   established algorithms to generate universally unique identifiers
   (UUID).  UUID version 4, described in Section 4.4 of [RFC4122],
   is RECOMMENDED.
END

— Section 4.1.4 —

   For example, it is not to be used to further the understanding of
   non-standard properties.

Just a suggestion here: I would specifically note that this hurts
interoperability, something such as this:

NEW
   For example, it is not to be used to further the understanding of
   non-standard properties, a practice that is knows to cause long-term
   interoperability problems.
END

— Section 4.1.6 —
As this is mandatory (and “created” is not), what is the value of this
property if the object has not been modified?  Is it the initial
creation date/time?  The text should be clear.

— Section 4.2.3 —

   They MAY
   define parameters and the "charset" parameter value MUST be "utf-8",

“define”?  Do you maybe mean “include”?

— Section 4.2.4 —

   Indicates the time is not important to display to the user when
   rendering this calendar object, for example an event that
   conceptually occurs all day or across multiple days, such as "New
   Year's Day" or "Italy Vacation".

This is spliced, and also is long and awkward.  I suggest this:

NEW
   Indicates that the time is not important to display to the user when
   rendering this calendar object. An example of this is an event that
   conceptually occurs all day or across multiple days, such as "New
   Year's Day" or "Italy Vacation".
END

— Section 4.2.5 —

      This MUST be either one of the following values,

“Either” is one of two, and there are more than two here.  Just remove “either”.

I also don’t understand what it means for a JSCalendar object to start
or end at a particular location.  Does it mean that the event or task
described occurs at the location?  Where does “start” and “end” come
in?  Are we talking about things that can move, so they might start
one place and end at another?  So what happens if the event doesn’t
move?  Do we set both “start” and “end” to the same location?  Why is
there no way to specify intermediate locations?

Can you explain this better?

      A set of link ids for links to alternate representations of this

Make it “alternative” (as you do in the next paragraph).

      This MUST be omitted
      if none (rather than an empty set).

Make it, “If there are no links, this MUST be omitted (rather than
specified as an empty set).”

— Section 4.2.7 —

      This MAY be a "data:" URL

You might want a citation to RFC 2397 here.

      Links with a rel of "describedby" SHOULD be considered by the
      client to be an alternate representation of the description.

“alternative”

      Links with a rel of "icon" SHOULD be considered by the client to
      be an image that it MAY use when presenting the calendar data to a
      user.

I’m a bit confused by the SHOULD/MAY.  If the image is not used when
presenting to the user, is there any other thing that SHOULD be done
with it?

      "rel" property MUST be set to "icon".  The value MUST be either

Again, remove “either”.

      *  "badge": an image inline with the title of the object.

I don’t understand what “an image inline with the title” means.  Do
you mean, “an image meant to be displayed along with the title”?  Or
something else?

— Section 4.2.11 —

   value is a case-insensitive color name taken from the set of names

It doesn’t make sense to say “case-insensitive” unless you’re talking
about a comparison.  Do you mean to say that it’s a lower-case
representation?  An upper-case representation?  Something else?

— Section 4.3 —

   Some events and tasks occur at regular, or indeed irregular,
   intervals.

If you keep “indeed”, it needs commas both before and after it.
Better, just remove the word; it adds nothing.  And the commas after
“regular” and “irregular” are both unnecessary.

   Rather than having to copy the data for every occurrence,
   you can instead have a master event with a recurrence rule

While I don’t object to your using “you” here, it’s striking that you
use “you” exactly twice in the document (the other is in 4.3.2.1
bullet 2.1), so this is a style deviation.  Do as you see fit with
that information.

— Section 4.3.2 —

   If the task defines neither a "start" nor "due" date-time,
   its "recurrenceRules" property value MUST be null.

Or absent?

      This MUST be either a CLDR-registered calendar system
      name, or a non-standard, experimental calendar system name
      prefixed with the characters "x-".

1. CLDR?  Please define and include a reference citation.

2. See RFC 6648 and tell me why we should still have “x-“ here.

— Section 4.3.2.1 —

           the first day of the next month (if skip == "forward") or the
           last day of the current month (if skip == "backward").

In the previous bullet you say << (if skip is "backward”) >>, and I
suggest you write these that way as well.  Consistency.  (Similar
comment for << if skip == "omit" >> a few bullets down.)

— Section 4.4.1 —

   The priority is specified as an integer in the range 0 to 9.  A value
   of 0 specifies an undefined priority.  A value of 1 is the highest
   priority.  A value of 2 is the second highest priority.  Subsequent
   numbers specify a decreasing ordinal priority.  A value of 9 is the
   lowest priority.

What happens whenb priority 0 is mixed with other priority values?

— Section 4.4.4 —

   The value is a URI to use that
   method.

There’s no antecedent to “that”.  I think it should be, “The value is
a URI for the method specified in the key.”  (Same comment for Section
4.4.5.)

   property MUST be omitted if no method is defined (rather than an
   empty object).

“rather than being specified as an empty object”.  (Same comment for
Section 4.4.5.)

   o  "other": The organizer is identified by this URI but the method
      how to submit the RSVP is undefined.

Poor English here; make it, “the method for submitting the response is
undefined.”  (Same comment for Section 4.4.5.)

      *  "resource": a non-human resource, e.g. a projector

      *  "location": a physical location involved in the calendar object
         that needs to be scheduled, e.g. a conference room.

I would argue that a location is a non-human resource.  I suggest
switching the order and clarifying it, thus:

NEW
      *  "location": a physical location that needs to be scheduled, such
         as a conference room.

      *  "resource": a non-human resource other than a location, such as
         a projector
END

— Secton 4.4.5 —

      This MUST be omitted if none (rather than an empty set).

I’ve mentioned this enough times that you should know how to fix this
and all other similar sentences.

      object, which caused this participant to be invited due to their
      membership of the group(s).

“membership in the group(s).”

— Section 4.5.2 —

   o  trigger: "OffsetTrigger|AbsoluteTrigger|UnknownTrigger"
      (mandatory)

This is a manner of specifying this sort of thing that has not been
used before.  You’ve always been using a defined type.  Here (and in
other items below) you appear to be using specific strings with no
explanation about what the “|” means.  There’s an inconsistency here
that I’d like to see fixed or clearly explained.

      Defines when to trigger the alert.  New types may be defined in
      future RFCs.

“future documents” or “future specifications”, and similarly for other
instances of “future RFC”.

      The Relation object SHOULD set the "parent" relation type, but MAY
      be empty.

These are contradictory: if it SHOULD be non-empty, then it’s not
correct that it MAY be empty.

— Section 4.7.2 —

   Maps identifiers of custom time zones to their time zone definition.

Number agreement: “definitions”

— Section 6.8 —

   a virtual location.  Fans can see a live convert on premises or

“concert” ?

         "description": "Music Bowl, Central Park, New York",
         "coordinates": "geo:40.7829,73.9654"

Should this maybe be << geo:40.7829,-73.9654 >>

— Section 7 —

   The transmission of such information must be careful to protect it
   from possible threats, such as eavesdropping, replay, message
   insertion, deletion, modification, and man-in-the-middle attacks.

“must be done carefully”


— Section 7.1 —

   It is not always possible to tell this through static
   analysis of the rule, so implementations MUST be careful to avoid
   getting stuck in an infinite loop, or otherwise exhausting resources,
   searching for the next occurrence.

Number agreement: “infinite loops”
Also, “exhausting resources while searching”

— Section 8.1 —

   Person & email address to contact for further information:
      [e-mail]

The address is incorrect; it should be calsify, not calext.

— Section 8.2.3 —

   This preferably takes the form of an RFC, but for simple definitions
   a description in the registry may be sufficient.

I’ve had a conversation with IANA about this.  The operative bit here
is whether "a description in the registry" could serve as the
"specification".  I don't think we'd accept that.  It's supposed to
be, according to BCP 26, a "permanent and readily available public
specification, in sufficient detail so that interoperability between
independent implementations is possible."  While what you put in the
registry can be considered permanent, and it's certainly readily
available and public, it's a bit of a stretch. And it's hard to
imagine that a sentence or two in a registry could be "sufficient
detail".  I'd say that some document, however brief, that's external
to IANA is required.

   Before a period of 30 days has passed, the DE will either approve or
   deny the registration request and publish a notice of the decision to
   the Calext WG mailing list or its successor, as well as inform IANA.

IANA has expected response times, and specifying this stuff in RFCs is
not a good idea.  I’d suggest tagging this for IANA review and getting
agreement with IANA about what we should say here.

   If the DE does not respond within 30 days, the registrant may request
   the IESG take action to process the request in a timely manner.

This part definitely needs to go.  People can always take complaints
to the IESG, and we shouldn’t be saying that here.

   The IESG may reassign responsibility for a JSCalendar property.  The
   most common case of this will be to enable changes to be made to a
   registration where the author of the registration has died, moved out
   of contact, or is otherwise unable to make changes that are important
   to the community.

And similarly for this; please strike it.

— Section 8.2.5 —

      The property name MUST
      NOT already be registered for any of the objects listed in
      Context.

What does “listed in Context” mean?  Can you explain this in the document?

— Section 8.4.1 —

   o  Experts: The initial list of experts for Expert Review of updates
      to the subregistry.

Hm, are you actually proposing that registrations provide their own
lists of experts, rather than having them appointed by the IESG?  I
don’t think that’s going to work.  We probably need to have a
discussion about the intent here, so please initiate that by
explaining to me what you’re looking for.

— Section 10 —
It’s not acceptable that all references are informative: there are
clearly normative references here, and you’ll have to go through and
sort out which ones are normative (which ones are for material that’s
necessary for understanding this document.  At the very least, the
references for BCP 14, JSON, JSON Pointer, and things such as that
will be normative.

-- 
Barry