search

adlnet / xAPI-Spec

The xAPI Specification describes communication about learner activity and experiences between technologies.
https://adlnet.gov/projects/xapi/
908 stars 404 forks source link

1.0.3 communication edits #918

Closed andyjohnson closed 8 years ago

andyjohnson commented 8 years ago

Check the individual commits for comments. Nothing too drastic. I did change "call" to "request" in some places, which I thought more acceptable. As with anything in these, please point out if I did something wrong.

garemoko commented 8 years ago

Checked the first commit. Left a comment.

garemoko commented 8 years ago

Just realized that my comment is superceeded by a later commit.

Looking at that section at the end of 1.3, the paragraph starting "There might be cases where there is a requirement for" has a few problems:

garemoko commented 8 years ago

@andyjohnson finished review. All my comments are showing as being on an outdated diff and are hidden, but most/all of them are still relevant in the latest version.

andyjohnson commented 8 years ago

I moved some text in 1.3 and added a couple lines regarding the use of the section. I'm now claiming that support of IE9 and less is not required, but the requirements in the section are relevant IF they are supported. Since removing the section is also on the table (I wouldn't mind removing it), we have to be careful that we weren't indicating in previous versions that this syntax MUST be supported.

andyjohnson commented 8 years ago

Thanks @garemoko for the review. Well done as always. Can we get a 3rd pair of eyes on this to get it merged? Sorry I was slow on this.

garemoko commented 8 years ago

Added some more comments on the latest changes; still showing as on an outdated diff for some reason, but definitely relevant.

I haven't reviewed to check that all my earlier comments had been acted on.

andyjohnson commented 8 years ago

So to be clear (probably need a final discussion on the issue), the Section 1.3 on Cross Domain Scripting does indeed need to be supported by LRSs and we cannot remove the section or any of the requirements in version 1.0.X. All comments should be addressed. Was a bit rushed before to get the last round in. Thanks for the reviews @garemoko !

oconnetf commented 8 years ago

I tossed in a couple of very minor whitespace issues I noticed.

As long as @garemoko accepts your perspective on 1.3, I'm 👍.

garemoko commented 8 years ago

image

Pink arrow: A proxy is only needed IF you want to support HTTP to HTTPS requests from IE 9 or lower. You can do HTTP to HTTPS (or HTTPS to HTTP!) without a proxy if you use a modern browser.

Green arrow: paragraph spacing

garemoko commented 8 years ago

Also, RE the last sentence, the security risk isn't in mixing schemes, it's in using http at all. I think you missed this comment: https://github.com/adlnet/xAPI-Spec/pull/918#issuecomment-214516051

garemoko commented 8 years ago

Space added in response to https://github.com/adlnet/xAPI-Spec/pull/918#discussion_r60986479 but it's on the wrong side of the quote.

garemoko commented 8 years ago

Not sure if this is in scope of this PR or not but... image

garemoko commented 8 years ago

Might also be out of scope of this PR, but I'm not sure what the following paragraph adds or even what point it's trying to make:

As a result, Learning Record Consumers might receive responses that include Statements of different versions. The version header allows for these version differences to be handled correctly, and to ascertain that no partial or mixed LRS version implementations exist.

I propose we delete it and merge the remaining two sentences in 3.3 Versioning into a single paragraph.

Also, in that next sentence, can we change reliably know compatibility to remain interoperable?

garemoko commented 8 years ago

Aside from the comments just added, looks like all other comments have been actioned (or at least responded to in the case of capitalization). Thanks!

andyjohnson commented 8 years ago

Agreed on the paragraph getting deleted, made those changes. In regards to the bullet spacing, it is right down the middle in terms of usage/non-usage. It is something to look for in the final pass as it is in all documents. I'm learning towards using spaces as some of the bullet requirements can be entire paragraphs. I think we'll need a final quick look at the Cross-Domain/HTTP Security section as far as content, but I recommend merging this PR (I went with all of your suggestions) and tackling it separately. Thanks to you both, @garemoko and @oconnetf for reviewing this PR multiple times.

garemoko commented 8 years ago

Yeh, agree that the cross scheme paragraph needs more attention; (I didn't expect you to copy my words exactly and some of it is a little rough)

Honestly I'd prefer to tackle that in this PR but I don't mind handling it separately if you prefer so +1

andyjohnson commented 8 years ago

Thanks @garemoko - it frees me up to do other PRs and have peace of mind, so I'd rather merge it. I'm travelling now but should be back on this tomorrow.