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

[Barry Leiba <[e-mail]>]

Thanks for posting version -24.  I'll give that a look over as soon as
I can, and then get last call started on it.

Barry

On Tue, Feb 18, 2020 at 10:58 PM Barry Leiba <[e-mail]> wrote:
>
> 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