Hi Rich,
Thanks for the detailed review. I used you input to produce draft -18.
Comments inline.
On 1/29/24 6:06 PM, Rich Salz via Datatracker wrote:
Reviewer: Rich Salz
Review result: Ready with Nits
I am doing the Artart directorate review for the jmap-sieve document. This is
primarily for the appropriate ADs. Authors should consider this like any other
LC comments.
I am not an expert on jmap or sieve, and have moderate familiarity with json,
so caveat lector.
In my view this has nits that can be fixed without controversy as they are all
editorial.
I was surprised that the Abstract did not include more text from Section 1.
What text did you expect to see? I added a new sentence to the Abstract.
Sec 1, is 8620 the "core specification"? If so, it should be described as such
two paragraphs above when it is first used. An example linebreak could be
useful. I am surprised a positive indication, such as a trailing backslash,
isn't used.
The use of unqualified "core" appears in other JMAP RFCs, but I decided
to remove that wording and reference RFC 8620 directly.
Sec 1.1 and 1.2 could be combined.
Agreed and done.
Sec 1.3.1, I am completely befuddled by what "The value of this property in an
account's accountCapabilities property is an object that MUST contain the
following information on per-account server capabilities:" property is
mentioned. The above one, which is a string? Or some unnamed object which
contains the elements specified below it. Maybe it's just cut+paste
duplication. Consider a worked example.
It is kind of wordy, but similar wording appears in other JMAP RFCs and
I think those familiar with JMAP are able to parse it. That said, I have
added an example which hopefully clarifies what belongs in each part of
the capabilities.
Sec 2, I don't see why the requirements around the script "blob" are given in
the "blobId" field; they should be somewhere else.
Agreed. I have split those requirements into a separate sub-section.
The "isActive" wording is a
little clunky. Perhaps "At most one script may be used to filter incoming
messages, indicated by having its isActive field set. Keep the last sentence.
Yep. I used a slightly modified version of your suggested text.
Sec 2 all examples. Nice to see them. I cannot comment on their accuracy, I
assume the WG and other experts verified them. I think the parens around the
URL template should be removed. You can say, the examples below assume that the
URL template is "/host/...." etc and I think it is better to use backslash on
the continued line, but good that you are calling it out as the only wrapped
part. (It is the only wrapper part, right? If so, say so).
I have reworded the example descriptions to reference URLs in the
capabilities example, and have also tweaked the example so that the
request line no longer needs to be lone-wrapped.
--
Kenneth Murchison
Senior Software Developer
Fastmail US LLC
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
