Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial Notes merged into main text #9

Closed
wants to merge 15 commits into from
Closed

Editorial Notes merged into main text #9

wants to merge 15 commits into from

Conversation

hhalpin
Copy link

@hhalpin hhalpin commented Nov 30, 2015

In general, when we move to Proposed Recommendation status we want Editorial Notes removed. However, it seemed for most of these editorial notes that the point being made (such as BER encoding not being supported) was important and should be part of the spec. So removed the editorial notes and put it back into their text back into the main spec. text.

The two exceptions were 1) Updating to W3C DOM4 and 2) Changing text around algorithms being 'features at risk'.

References were updated to normative versions. Added also (to resolve Akamai formal objection) was the sentence in this email (https://lists.w3.org/Archives/Public/public-webcrypto/2015Oct/0040.html) and a reference to the CFRG note on security guidelines.

Of course, the final call on this should be that of the editors.

@sleevi
Copy link
Contributor

sleevi commented Nov 30, 2015

I appreciate the enthusiasm, but there's issues with nearly every one of these.

It would be great if you'd break down the problem to the smallest possible problem set, and submit individual PRs for treating with that. For example, many of these commits should arguably be as separate PRs, so that we can treat them each with the attention they need.

I've left a number of comments, but considering that a significant number of these changes are incorrect, and we'd want to split it up, it'd be great to continue via new PRs.

@sleevi sleevi closed this Nov 30, 2015
@hhalpin
Copy link
Author

hhalpin commented Nov 30, 2015

Ryan,

I am merely pointing out, as promised, everything that needs to be changed for us to go to PR. Please use your Editor's hat and fix these to the best of your ability.

According to W3C process, in order to move to Proposed Recommendation we need to do the following:

  1. All normative references in a spec should be to other normative references (although we can be looser with informative references). If you want to call it "JWK" rather than "RFC 7517", that's fine. However, the reference needs to be the normative document, not a 'work in progress' that is out of date.

  2. Editorial Notes usually mean 'bugs we are still fixing'. We need them all resolved. If you think the text is inappropriate, please change or remove your editorial note text with whatever text you think is spec-quality.

  3. Re the CFRG draft, yes, it's been adopted as an "active RG" document, which is all that is needed for an informative reference.
    ttps://datatracker.ietf.org/doc/draft-irtf-cfrg-webcrypto-algorithms/

So just make the needed changes this pull request noted ASAP so we can move to Proposed Recommendation - and then we can have a bottle of virtual champagne!

Once you've made the changes, I'll move the final document to /TR/ staging so its published at w3.org and not just github. OK?

@hhalpin hhalpin reopened this Nov 30, 2015
@sleevi
Copy link
Contributor

sleevi commented Dec 1, 2015

However, the reference needs to be the normative document, not a 'work in progress' that is out of date.

I suspect you're on an outdated version - @jimsch fixed this in c5c7460

Editorial Notes usually mean 'bugs we are still fixing'

Except if you read through them, you can see that they aren't that, nor is it appropriate to just merge them in. I'm aware of the need to resolve them, I'm more than happy that you're willing to take the time to contribute, but I was pointing out the problems with your approach. If you're not confident you'd be able to address the substance, then I think that's fine, we leave it as an editorial cleanup. But it does mean that we should abandon this PR. My hope was that, by encouraging you to split into smaller PRs, we can have more meaningful discussions and empower you to make those changes yourself :)

So just make the needed changes this pull request noted ASAP

As indicated, this will take time. I highlighted the issues with your approach, and it's now unclear whether you intend to address them in this/separate PRs, or if you'd prefer the Editors or other members in the WG do it. Just wanting to make sure we're not in a situation where even more work is being created - or unnecessary work is being done.

@hhalpin
Copy link
Author

hhalpin commented Dec 1, 2015

@sleevi It seems your preferred workmode is I do separate pull requests for each change (or reasonably coherent group of changes, such as the same editorial note text spread out in a few places or same reference) and do the commit to the XML, correct?

That's no problem - I'll do those changes tomorrow. However, if you just want to make the edits you think fix these open editorial notes, I think the WG would also accept your changes.

The key is we want to move to PR in two weeks, so we need to push this out ASAP. I'm happy to extend the PR transition a bit if editorial work takes more time, but just not indefinitely :)

@mwatson2
Copy link
Collaborator

@hhalpin What is this PR for ? Do we need it ? It seems to be against only the Overview.html, not the Overview-WebCrypto.xml.

@@ -14683,6 +14661,7 @@
<li>JOSE Implementation Requirements: Prohibited</li>
<li>Change Controller: W3C Web Cryptography Working Group</li>
<li>Specification Document(s): [[ This Document ]]</li>
<li>Algorithm Analysis Document(s): n/a </li>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a strong preference that this line not be in the template. They are not required by the registration template and need only be provided if asked for. However the value of "n/a" is clearly not a correct answer. If requested by the expert reviewer, then it might need to be supplied.

In these cases it would be something about how this is a totally broken algorithm.

@mwatson2
Copy link
Collaborator

@hhalpin I think this PR has been superseded by recent changes. Could you confirm ?

Copy link

@engelke engelke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issues with this PR.

@mwatson2
Copy link
Collaborator

This is an update to the generated Overview.html for the last publication milestone. I believe it is completely out of date with respect to our current draft, which contains no Editor's notes.

@mwatson2 mwatson2 closed this Oct 18, 2016
@plehegar plehegar deleted the pr-edits branch May 14, 2018 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants