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

[Barry Leiba <[e-mail]>]

All this looks good, and thanks.  Post the update when ready.

On your question:

> other-param (and xparam) are defined in 5545. Do we require an
> explicit reference to that spec.

I think “xparam” is not defined there (“x-param” is).  And I guess we don’t
have to be explicit, but I think it would help to have a comment that says
where it’s defined, or perhaps a general comment that some items are
defined in 5545.

Barry

On Thu, Dec 31, 2020 at 4:11 PM Michael Douglass <[e-mail]>
wrote:

> Thanks for your comments: I'll work through them below.
>
> I'll submit an update in a short while.
> On 11/19/20 14:08, Barry Leiba wrote:
>
> 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.
>
> Replaced with
>
> Grouped iCalendar:  iCalendar entities can be related to each other
>    as a group.  CATEGORIES are often used for this purpose but are
>    problematic for application developers due to their lack of
>    consistency and use as a free-form tag.
>
>    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?
>
> How about the following:
>
>
> — 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”.
>
> Yes - done
>
> — 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
>
> Thank you - done
>
> — 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.
>
> Is this better?
>
> The term "concept" more accurately defines what we often
> mean by a category. It's not the text string that is important
> but the meaning attached to it. For example, the term
> "football" can mean very different sports.
>
> 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
>
> Done.
>
> — Section 1.4 —
>
>    It is often necessary to relate calendar components.
>
> To what?  To each other?  Something else?  Please say.
>
> Does this work?
>
> It is also often necessary to reference calendar components
> in other collections. For example, a VEVENT might refer to
> a VTODO from which it was derived.
>
>
> — Section 1.6 —
> Please use the new BCP 14 boilerplate and add a normative reference to RFC 8174.
>
> Done.
>
> — 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”?
>
> Changed to:
>
> This allows for linking within a single collection and the value
> MUST be 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.
>
> Done
>
> — 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
>
> Done
>
> — 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).
>
> Done
>
> 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).
>
> I ended up reworking this section.
>
> First I realized I didn't have to redefine the parameter anyway - just
> define new values. I broke it into 2 new sections - one defining the
> temporal values and one the rest - simply because the temporal values
> seemed to justify their own section.
>
>       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”
>
> done
>
>       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.)
>
> I'll take a read through looking for that
>
> — 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”.
>
> Thanks
>
> — 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?
>
> I have a copy of their categorizations... - but I'll cut it out of this
> spec
>
>      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
>
> Agreed
>
>      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?)
>
> Yes and no - removed 2
>
> — 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.
>
> OLD
>
> link            = "LINK" linkparam  /
>                   (
>                     ";" "VALUE" "=" "TEXT"
>                     ":" text
>                   )
>                   (
>                     ";" "VALUE" "=" "REFERENCE"
>                     ":" text
>                   )
>                   (
>                     ";" "VALUE" "=" "URI"
>                     ":" uri
>                   )
>                   CRLF
>
>
> 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)
>
>                 )
>
> NEW
>
> link           = "LINK" linkparam ":"
>                    ( text /  ; for VALUE=REFERENCE
>                      uri /  ; for VALUE=URI
>                      text ) ; for VALUE=TEXT
>                  CRLF
>
> linkparam      = ; the elements herein may appear in any order,
>                  ; and the order is not significant.
>
>                  (";" "VALUE" "=" ("UID" /
>                                    "URI" /
>                                    "TEXT"))
>                  1*(";" linkrelparam)
>                  (";" fmttypeparam)
>                  (";" labelparam)
>                  *(";" other-param)
>
> END
>
> Also added paragraph providing reference to labelparam
>
> Also, this section is exactly one that *can* benefit from multiple
> examples, each showing a different set of value types and parameters.
>
> Yes. Additionally, the example is incorrect. The abnf doesn't allow for a
> default value type so it should specify VALUE=URI
>
> Also , "The Egg" is a real place. Changed to "The Venue".
>
> — 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
>
> Done.
>
>      refidparam      = *(
>
>                      ; the following is OPTIONAL
>                      ; and MAY occur more than once
>
>                      (";" xparam)
>
>                      )
>
> Changed to
>
> refidparam      = *(";" other-param)
>
> Same comment as with conceptparam.  And, again, where is xparam defined?
>
> other-param (and xparam) are defined in 5545. Do we require an explicit
> reference to that spec.
>
> — 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.
>
> done. I also added the following paragraph to the description:
>
> To preserve backwards compatibility the value type MUST be UID
> when the PARENT, SIBLING or CHILD relationships are specified.
>
> 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.
>
> Changed the section 5.1 definition to be linkrelparam and changed its name
>
> For related:
>
> OLD
>
> related    = "RELATED-TO" relparam ( ":" text ) /
>              (
>                ";" "VALUE" "=" "UID"
>                ":" uid
>              )
>              (
>                ";" "VALUE" "=" "URI"
>                ":" uri
>              )
>              CRLF
>
> relparam   = *(
>             ;
>             ; The following are OPTIONAL,
>             ; but MUST NOT occur more than once.
>             ;
>             (";" reltypeparam) /
>             (";" gapparam) /
>             ;
>             ; The following is OPTIONAL,
>             ; and MAY occur more than once.
>             ;
>             (";" other-param)
>             ;
>             )
>
> NEW
>
> related    = "RELATED-TO" relparam ":"
>              ( uid /  ; for VALUE=UID
>                uri /  ; for VALUE=URI
>                text ) ; for VALUE=TEXT or default
>              CRLF
>
> relparam   = ; the elements herein may appear in any order,
>              ; and the order is not significant.
>              [";" "VALUE" "=" ("UID" /
>                                "URI" /
>                                "TEXT")]
>              [";" reltypeparam]
>              [";" gapparam]
>              *(";" other-param)
>
> END
>
>
>