[calsify] AD review of draft-ietf-calext-ical-relations-05

[Barry Leiba <[e-mail]>]

Hi, Michael and the working group,

As we discussed the ical-relations doc during the IETF 109 session, I
though I'd get a jump on the publication request and do an AD review
now.  Here it is below.

— Section 1 —

   associated meta-data.  These relationships can take the following
   forms

Nit: That needs a colon at the end of it.

      CATEGORIES are often used for this purpose but are
      problematic for application developers.

It’s a small thing, but it might be good to briefly say why it’s problematic.

   Linked:  Entities are linked to each other through typed references.

Can you be a little less terse here, or explain what you mean by
“typed references” in this context?

— Section 1.1 —

   and lag values and RELATED-TO is extended to allow URI values.

This particular sentence would benefit from the Oxford comma after “lag values”.

— Section 1.2 —

   This may be, for example, to identify
   the tasks associated with a given project without having to
   communicate the task structure of the project, or, for example, in a
   package delivery system all tasks associated to a specific package.

Another small suggestion for rephrasing (to avoid the awkward
repetition of “for example”):

NEW
   Two examples of how this may be used are to identify the tasks
   associated with a given project without having to communicate the
   task structure of the project, and to group all tasks associated to
   a specific package in a package delivery system.
END



— Section 1.3 —

   This more accurately
   defines what we mean by a category.  It's not the words but the
   meaning.

Two things here:
1. I’m not sure what “this” refers to.  Is it “the name CONCEPT”?  Or
is it the Simple Knowledge Organization System?
2. The sentence “It's not the words but the meaning,” is just sort of
hanging around without being connected to anything.

I’d really like to see this paragraph reworded.

   Rather than attempt to add semantics to the current property it
   seeems best to continue its usage as an informal tag and establish a
   new property with more constraints.

I think this would be clearer with names on the properties (and one
"e" fewer in "seems"):

NEW
   Rather than attempt to add semantics to the existing CATEGORY
   property, it seems best to continue its usage as an informal tag
   and to establish a new CONCEPT property with more constraints.
END

— Section 1.4 —

   It is often necessary to relate calendar components.

To what?  To each other?  Something else?  Please say.

— Section 1.6 —
Please use the new BCP 14 boilerplate and add a normative reference to RFC 8174.

— Section 2 —

   UID:  This allows for linking within a single collection and the
      value is assumed to be another component within that collection.

Why “assumed to be”?  Is this any different to “the value is the UID
of another component within that collection”?

For “xpointer”, please be consistent in capitalizing the term.  I
suggest using “XPointer” here and in Section 6, to match usage in the
reference doc.

— Section 3 —

   [RFC5988] defines two forms of relation type, registered and
   extension.  Registered relation types are added to a registry defined
   by [RFC5988] while extension relation types are specified as unique
   unregistered URIs, (at least unregistered in the [RFC5988] registry).

First, RFC 8288 replaced 5988 three years ago; please change to using
that as the reference.

Second, the parenthetical seems odd.  I suggest, “while extension
relation types are specified as unique URIs that are not registered in
the registry.”

Third, I strongly suggest adding section references here.  So the
result might be this:

NEW
   [RFC8288] defines two forms of relation type: registered and
   extension.  Registered relation types are added to the Link
   Relations registry as specified in Section 2.1.1 of [RFC8288].
   Extension relation types, defined in Section 2.1.2 of [RFC8288],
   are specified as unique URIs that are not registered in the registry.
END

— Section 4 —

   Relationship parameter type values are defined in section 3.2.15. of
   [RFC5545].

Nit: remove the dot at the end of “3.2.15” (and capitalize “Section”,
to be consistent with how we do section references).

This is probably a good time to pay attention to BCP 178 (RFC 6648) by
removing the x-name construction from the ABNF (also in Section 5.1).

      The GAP parameter specifies the
      lead or lag time between the predecessor and the successor.

This would benefit from a forward reference: “The GAP parameter,
defined in Section 5.2, specifies”

      the description of each temporal relationship below we refer to
      Task-A which contains and controls the relationship and Task-B the
      target of the relationship.

Nit: this needs commas: “Task-A, which contains and controls the
relationship, and Task-B, the target”.  (There are a lot of missing
commas throughout, and I’m not calling them all out; we’ll leave most
to the RFC Editor.)

— Section 5.2 —

   Purpose:  To specify the length of the gap, positive or negative
      between two temporaly related components.

Another example of a missing comma that rates mention: after “negative”.

— Section 7.1 —

      Possible category resources are the various proprietary systems,
      for example Library of Congress, or an open source derived from
      something like the dmoz.org data.

dmoz.org died 3.5 years ago; do we really still want to use it as an example?

     conceptparam = *(
                   ;
                   ; The following is OPTIONAL,
                   ; and MAY occur more than once.
                   ;
                   (";" other-param)
                   ;
                   )

The ABNF already says that it can occur zero or more times, so the
comment is supefluous.  I think the comments and the nested grouping
makes this more confusing than helpful, and suggest just changing it
to this:

NEW
     conceptparam = *(";" other-param)
END

     SCONCEPT:http://example.com/event-types/sports
     CONCEPT:http://example.com/event-types/arts/music
     CONCEPT:http://example.com/task-types/delivery

Is “SCONCEPT” a typo?  (And is there really value in three essentially
indistinguishable examples?)

— Section 7.2 —

     link            = "LINK" linkparam  /
                       (
                         ";" "VALUE" "=" "TEXT"
                         ":" text
                       )
                       (
                         ";" "VALUE" "=" "REFERENCE"
                         ":" text
                       )
                       (
                         ";" "VALUE" "=" "URI"
                         ":" uri
                       )
                       CRLF

This ABNF doesn’t make any sense.  Are you missing some alternative operators?

     linkparam       = *(

                     ; the following is MANDATORY
                     ; and MAY occur more than once

                     (";" relparam) /

                     ; the following are MANDATORY
                     ; but MUST NOT occur more than once

                     (";" fmttypeparam) /
                     (";" labelparam) /
                     ; labelparam is defined in ...

                     ; the following is OPTIONAL
                     ; and MAY occur more than once

                     (";" xparam)

                     )

And there are multiple problems here:
- Where is labelparam defined?
- Where is xparam defined?
- All elements of linkparam are optional, because you use “*”, so how
can any elements be MANDATORY?
- The ABNF should make it clear, without comments, whether things are
optional or mandatory and whether they can appear once or multiple
times.

Please re-work this ABNF, and get help from an ABNF expert if you need that.

Also, this section is exactly one that *can* benefit from multiple
examples, each showing a different set of value types and parameters.

— Section 7.3 —

   Description:  The value of this property is a text identifier that
      allows associated components to be located or retrieved as a
      whole.  For example all of the events in a travel itinerary would
      have the same REFID value.

This seems to be more vague than necessary.  May I suggest an update?:

NEW
   Description:  The value of this property is free-form text that
      creates an identifier for associated components.  All components
      that use the same REFID value are associated through that value
      and can be located or retrieved as a group.  For example, all of
      the events in a travel itinerary would have the same REFID value,
      so as to be grouped together.
END


     refidparam      = *(

                     ; the following is OPTIONAL
                     ; and MAY occur more than once

                     (";" xparam)

                     )

Same comment as with conceptparam.  And, again, where is xparam defined?

— Section 8.1 —

      definition here extends the definition in Section 3.8.4.5. of
      [RFC5545]

Also here: remove the dot at the end of the section number.

For the ABNF in this section I have the same comments as for the ABNF
in Section 7.1.  In addition, this defines “relparam”, but “relparam”
is already defined in Section 5.1.  That needs to be fixed.

-- 
Barry