search

mlichvar / draft-ntp-ntpv5

I-D: Network Time Protocol Version 5
4 stars 5 forks source link

Comments and some suggested corrections #2

Open sjvudp opened 1 year ago

sjvudp commented 1 year ago

Here are my comments and a few suggested edits. See the XML comments in the document!

mlichvar commented 1 year ago

I pushed your suggestions to the repo. I'll go through your comments and try to address the easier one. I guess we can keep the discussion on the WG list for now and if it will be adopted we can handle each issue individually here on github.

sjvudp commented 1 year ago

The main purpose of filing a pull request is easier integration, and you'll also see who contributed what (git blame). And still with all those comments you can still process the output without visible changes.

mlichvar commented 1 year ago

If you submit a pull request for each issue, that will be easier to comment and work with. I could merge them with no changes in any order as we can reach an agreement. With multiple unrelated issues in one pull request it's difficult or slower to work with.

Comments about changes in the document should be separate from the source code. I don't think it's common for developers to discuss changes over commits. That would be a mess.

sjvudp commented 1 year ago

Hi!

I must agree that I have little experience with processing pull requests. Can you "pull into an integration branch", and then add chunks interactively from that integration branch? I specifically created a separate branch for my comments.

Regards, Ulrich

Miroslav Lichvar @.> schrieb am 26.09.2022 um 08:47 in Nachricht @.>: If you submit a pull request for each issue, that will be easier to comment and work with. I could merge them with no changes in any order as we can reach an agreement. With multiple unrelated issues in one pull request it's difficult or slower to work with.

Comments about changes in the document should be separate from the source code. I don't think it's common for developers to discuss changes over commits. That would be a mess.

-- Reply to this email directly or view it on GitHub: https://github.com/mlichvar/draft-ntp-ntpv5/pull/2#issuecomment-1257556153 You are receiving this because you authored the thread.

Message ID: @.***>

mlichvar commented 1 year ago

I'm not sure how would that work. With multiple changes in one commit it's more likely there will be a conflict. Ideally, each logical change in the document should have a separate issue/PR.

For an example, see the closed issues and pull request for the NTS document here: https://github.com/dfoxfranke/nts

sjvudp commented 1 year ago

You could extract my branch into a separate workspace, then checkout your branch, copy my file over to your branch and then do a "git add --interactive" using "patch". Then you could "git stash -k" the rest "away". At some later time you could "git stash pop" again, restoring the uncommitted changes and repeat at "git add --interactive"...

I see that this is not what you had in mind, but making a commit for each comment seems like overkill in this early stage of the draft. Sorry for the inconvenience.

mlichvar commented 1 year ago

I think we can address the issues you point out, but it might be too soon to be working on such details yet. I'm trying to get the draft adopted first and focus on the technical side like the loop detection.

mlichvar commented 1 year ago

I pushed another commit addressing some of the issues you raised.