Subject: [calsify] Early AD review of draft-ietf-calext-extensions-02.txt

From: Alexey Melnikov <[email-masked]>
Date: Wed, 1 Jun 2016 13:14:50 +0100

In general the document is well written. I do have some comments which I 
would like to discuss before moving this document forward:

Please change "/=" to "/=" everywhere in ABNF fragments. Only the latter 
is valid according to RFC 5234.

5.6.  CATEGORIES Property

    Conformance:  This property can be specified multiple times in an
       iCalendar object.

Can you clarify handling of multiple instances of this property versa a 
single instance with multiple values in it? Are the values additive?

In 5.10: The ABNF doesn't seem to be correct:

>    image      = "IMAGE" imageparam ( ":" uri ) /
Which alternatives does "/" separate? I think you are missing an extra 
() somewhere here.
>                 (
>                   ";" "ENCODING" "=" "BASE64"
>                   ";" "VALUE" "=" "BINARY"
>                   ":" binary
>                 ) /
Extra "/" not needed?
>                 CRLF
>
>    imageparam = *(
>                  ;
>                  ; The following is REQUIRED,
>                  ; but MUST NOT occur more than once.
>                  ;
>                  (";" "VALUE" "=" "URI") /
>                  ;
>                  ; The following is OPTIONAL for a URI value,
>                  ; RECOMMENDED for a BINARY value,
>                  ; and MUST NOT occur more than once.
>                  ;
>                  (";" fmttypeparam) /
>                  ;
>                  ; The following are OPTIONAL,
>                  ; and MUST NOT occur more than once.
>                  ;
>                  (";" altrepparam) / (";" displayparam) /
fmttypeparam is listed only here and not for VALUE=BINARY. Your text in 
this section suggests that this parameter should apply to both alternatives.
>                  ;
>                  ; The following is OPTIONAL,
>                  ; and MAY occur more than once.
>                  ;
>                  (";" other-param)
>                  ;
>                  )

As per above, there are multiple errors above, so I am not sure about 
the desired syntax.

In 5.11: Please mention "https" URI, HTTPBIS is phasing out non TLS 
protected HTTP access.

In 6.1:

>    displayval =   ("BADGE" /    ; image inline with the title of the
>                                 ; event
>                    "GRAPHIC" /  ; a full image replacement for the event
>                                 ; itself
>                    "FULLSIZE
Missing terminating " above.
> /  ; an image that is used to enhance the
>                                 ; event
>                    "THUMBNAIL / ; a smaller variant of "FULLSIZE" to be
Missing terminating " here as well.
>                                 ; used when space for the image is
>                                 ; constrained
>                    x-name /     ; Experimental type
>                    iana-token)  ; Other IANA registered type
>                                 ;
>                                 ; Default is BADGE

---

From: Cyrus Daboo <[email-masked]>
Date: Thu, 02 Jun 2016 12:11:38 -0400

Hi Alexey,
Thanks for your review comments.

> is valid according to RFC 5234.

Fixed.

> single instance with multiple values in it? Are the values additive?

Fixed by adding the following:

   Description:  When multiple properties are present, the set of
      categories that apply to the iCalendar object are the union of all
      the categories listed in each property value.

> Which alternatives does "/" separate? I think you are missing an extra ()
> somewhere here.
>>                 (

Fixed.

> this section suggests that this parameter should apply to both

The ABNF here is trying to following the syntax used for ATTACH 
(<https://tools.ietf.org/html/rfc5545#section-3.8.1.1>). Now that I have 
properly examined ABNF's operator precedence I see that "Concatenation" 
takes precedence over "Alternative". So 5545s "a b /c" construct should 
have been "a (b /c)".

I will fix the grouping.

> In 5.11: Please mention "https" URI, HTTPBIS is phasing out non TLS

Fixed. I changed all the examples to use https: instead of http: too.

> Missing terminating " here as well.

Both fixed.

I will post an updated draft later today.

-- 
Cyrus Daboo