Subject: Re: [apps-discuss] draft-wilde-xml-patch-08

From: Tony Hansen <[email-masked]>
Date: Tue, 25 Mar 2014 11:53:14 -0400

This is a review of draft-wilde-xml-patch-08.

Document: draft-wilde-xml-patch-08.txt
Title: A Media Type for XML Patch Operations
Author: Erik Wilde
Reviewer: Tony Hansen
Review Date: March 25, 2014

Some minor issues are described below that should be considered. This 
document is otherwise ready to be published.

Please resolve these comments along with any other comments you may receive.

     Tony Hansen

Major issues: none

 >>>> Nit. Abstract. Consider changing this sentence as indicated.

    The XML Patch media type "application/xml-patch+xml" defines an XML
    document structure for expressing a sequence of patch operations that
    are applied to an XML document.
to
    The XML Patch media type "application/xml-patch+xml" defines an XML
    document structure for expressing a sequence of patch operations >>>to
    be<<< applied to an XML document.

 >>>>Nit. Abstract. This sentence needs to be split apart. I suggest 
just changing the comma after "RFC 5261" to a period.

    " The XML Patch document format's
    foundations are defined in RFC 5261, this specification defines a
    document format and a media type registration, so that XML Patch
    documents can be labeled with a media type, for example in HTTP
    conversations."

 >>>>Minor. Abstract.

Actually, on rereading the abstract, I'm thinking it would be better 
expressed like the following. The primary focus of the I-D is the 
document format itself. The media type registration is a secondary 
consideration. In the following, I tried not to change your points, just 
the ordering and emphasis.

    The XML Patch document format defines an XML
    document structure for expressing a sequence of patch operations to
    be applied to an XML document.  The XML Patch document format
    builds on the foundations defined in RFC 5261. This specification 
defines the
    also provides the media type registration "application/xml-patch+xml",
    to allow the use of XML Patch documents in, for example, HTTP 
conversations.

 >>>>Nit: More white space would be useful around the XML segments, to 
offset them more.

 >>>>Minor: I STILL feel that section 2.2 would read better if it shows 
a before and after example of applying the principle described there.

For example, after the second paragraph in section 2.2, add something 
like the following. (I think I got the conversion right below.)

vvvvvv ====== vvvvvv
For example, consider the patch example in RFC 5621 Appendix A.1. Adding 
an Element. The patch
is shown there as an XML diff document:

    <?xml version="1.0" encoding="UTF-8"?>
    <diff>
      <add sel="doc"><foo id="ert4773">This is a new child</foo></add>
    </diff>

With XML Patch, this would be converted to

    <?xml version="1.0" encoding="UTF-8"?>
    <p:patch xmlns="http://example.com/ns1" xmlns:p="urn:ietf:rfc:XXXX">
      <p:add sel="doc"><foo id="ert4773">This is a new child</foo></p:add>
    </p:patch>
^^^^^^ ====== ^^^^^^

 >>>>Nit: Appendix A

"It described" => "It describes"

 >>>>Nit: Appendix A.1

"XMPL Patch" => "XML Patch"

 >>>>Minor. Appendix C
There needs to be a pointer to the ABNF spec, RFC 5234, where things 
like DIGIT and DQUOTE are defined.
I did not review in detail the ABNF grammar itself.

---

From: Erik Wilde <[email-masked]>
Date: Tue, 25 Mar 2014 10:06:18 -0700

hello tony.

> This is a review of draft-wilde-xml-patch-08.

thanks a lot for the review! i have just posted an updated version of 
the draft (http://tools.ietf.org/html/draft-wilde-xml-patch-09), and 

> the ordering and emphasis.

thanks for the suggestion, and i think your text is an improvement. i 
have changed the abstract as suggested.

>  >>>>Nit: More white space would be useful around the XML segments, to

i agree, but that's more a problem of xml2rfc which seems to have 
problems formatting <figure><artwork> properly. for now, i'll leave it 
as it is (since the XML source is correct), and then this can be 
addressed in the final production stage.

> a before and after example of applying the principle described there.

i have added the examples as suggested, showing all three documents 
(target document, patch document, result document). the patch document 
is a bit different than the one shown in RFC 5261, because it uses a 
different document element and a namespace.

> "XMPL Patch" => "XML Patch"

fixed.

> like DIGIT and DQUOTE are defined.

i have added a reference to RFC 5234, and added a sentence pointing out 
that DIGIT and DQUOTE are used as defined there.

thanks again and cheers,

dret.

-- 
erik wilde | mailto:[email-masked]  -  tel:+1-510-2061079 |
            | UC Berkeley  -  School of Information (ISchool) |
            | http://dret.net/netdret http://twitter.com/dret |

---

From: Tony Hansen <[email-masked]>
Date: Tue, 25 Mar 2014 14:07:38 -0400

Wow, fast response. :-)

One nit in the abstract, which turns out to be my fault in my previously 
suggested text:

    This specification defines the also provides
    the media type registration "application/xml-patch+xml", to allow the
to
    This specification >>><<< also provides
    the media type registration "application/xml-patch+xml", to allow the
or
    This specification >>><<< also defines
    the media type registration "application/xml-patch+xml", to allow the

This one can be fixed in AUTH48 if necessary.

Everything else looks fine with me.

     Tony

On 3/25/14, 1:06 PM, Erik Wilde wrote:
> i have added a reference to RFC 5234, and added a sentence pointing 
> out that DIGIT and DQUOTE are used as defined there.
>
> thanks again and cheers,
>
> dret.
>

---

From: Erik Wilde <[email-masked]>
Date: Tue, 25 Mar 2014 16:33:23 -0700

hello tony.

>     the media type registration "application/xml-patch+xml", to allow the

many thanks for checking again! 
https://github.com/dret/I-D/commit/472df96e19f661edfaedb23eeccab5a5615802ad 
fixes this issue, and i will use this either for a -10 draft, or as the 
starting point for the final publishing process.

thanks again,

dret.