Some intial read-through comments

[danielpeintner]

Find below some aspects we should discuss...

Abstract

• link to WoT Architecture points to CR, should be TR
  (Q: Should we point to the latest document? https://www.w3.org/TR/wot-architecture/ OR to dated URI ?)
• link to TD points to CR, should be TR
  (Q: Same as for Architecture)
• Editor's note: "plug-fests" .. use "Plugfest" everywhere else...

F. References

• Updates for
  • [WOT-ARCHITECTURE] --> Link+Date
  • [WOT-PROTOCOL-BINDINGS] --> Date
  • [WOT-SECURITY-BEST-PRACTICES] Broken link
  • [WOT-SECURITY-TESTING] Broken link
  • [WOT-TD] --> Link+Date

Minor updates/issues provided by PR

#229

[egekorkan]

Please find my review in the attached pdf. I have read until the chapter 7 and will read the rest later but I wanted to send it as soon as possible anyways.

I have put mostly notes, replace text, insert text, strikethrough and I have underlined some text that I see "suspicious", like ref errors
ScriptingAPI-review-egekorkan.pdf

[danielpeintner]

checking Ege's PDF chapters:
@zolkis Chapter 1-2
@relu91 Chapter 3-5
@danielpeintner Chapter 6

[danielpeintner]

@egekorkan others please provide comments in Github index.html (line by line commenting possible?)
Thanks!

[egekorkan]

I was thinking of the best way to do it as well. Should I create a PR with my comments that will be never merged?

[zolkis]

We can handle the PDF for now.
But in general, one can view the index.html and comment on each line.
Also, one can choose any commit and comment on that, creating new issues that refer to those comments.
Also, a WoT WG member can create a PR with trivial or non-trivial fixes.

[egekorkan]

So the problem is that I cannot comment on any file on the repository. The comment features become available when there is a PR or if I choose a certain commit and comment on that. Since it was a general review of the spec and not a single commit, I had find some other solution. Choosing a certain commit is also not good for reviewing the whole document since I cannot see the whole document on a commit that addresses a single section (most commits are like that).
I would like to make your work the easiest and would abide by any method :) For now one of three methods seem plausible:

• Continue with PDF. It is admittedly not the easiest way :/
• Create comments inside the html file via html comments and show them via a PR. This can also contain trivial fixes and until all my comments are resolved (by me deleting them via a commit that removes those comments), the PR can stay open.
• Creating issue(s) with my comments

[egekorkan]

Another alternative would be to use google docs. Here is one where I also added a comment and a review. The comments can be also linked individually and marked as resolved. The only disadvantage I see is that the document is on my own account and may not be available in 5 years.

[zolkis]

Google Doc would work, but no history in github for the conversation, indeed. We could manage that if we created issues out of the non-trivial discussions. That is one way to go.

For general review, I would prefer meta-PR, like this one: #232

However, for some of the comments the original method I proposed is better:
just view the file, click on line number, click on the 3 dots icon, choose "copy permalink" or "reference in new issue".
We should not be afraid of filing issues :).

[zolkis]

Here is one way that would work (also see the comments):
http://astrofrog.github.io/blog/2013/04/10/how-to-conduct-a-full-code-review-on-github/

[egekorkan]

Here is one way that would work (also see the comments):
http://astrofrog.github.io/blog/2013/04/10/how-to-conduct-a-full-code-review-on-github/

I would like to do it this way, shall I proceed?

[egekorkan]

Method above seems to work, I will put my comments here: https://github.com/egekorkan/wot-scripting-api/pull/1/files#diff-eacf331f0ffc35d4b482f1d15a887d3b . It might make more sense to open this empty branch in this repository such that the PR appears here?

[danielpeintner]

I think once #276 is merged we can close this is issue w.r.t. my points

[danielpeintner]

PR #276 merged