weixin_39707612
weixin_39707612
2020-12-30 06:23

diffscuss comments should be diff comments

As discussed on hacker news (https://news.ycombinator.com/item?id=7973832), many people feel that diffscuss should overload diff comments for its own purposes, rather than inventing its own syntax and breaking compatibility with the diff format. The only reason that was given against embedding diffscuss markup inside diff comments is that it would confuse the diffscuss parser in case unrelated comments are already present in the diff; this is easily solved by using a special beginning-of-line character inside diff comment blocks. In the proposed new format, "normal" diff comments would start with #, while diffscuss comments would start with #%* and #%-. In order not to break compatibility with existing diffscuss files, diffscuss (and its plugins) can still support the old syntax in read-only mode, but should use the new one by default when generating diffs.

该提问来源于开源项目:tomheon/diffscuss

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

12条回答

  • weixin_39853892 weixin_39853892 4月前

    Yeah, this is a strong argument, starting to lean towards it. Let me get the temp of the other guys on the project.

    点赞 评论 复制链接分享
  • weixin_39678426 weixin_39678426 4月前

    If there is an "import to diffscuss" step, you could escape pre-existing comments in an unambiguous, reversible way. The diffscuss comment removal step, while a trivial grep -v now, should probably be abstracted behind a command for most users anyway, so you can transparently restore the original comments with un-escaping there.

    Downside is possibly adding another step to the workflow (although maybe not at this point, since it's tightly integrated with git, which you can probably assume will have no existing diff comments).

    点赞 评论 复制链接分享
  • weixin_39853892 weixin_39853892 4月前

    All right, chatted this out, brief recap: - From the point of view of how we (Hut 8) use diffscuss, which is as our actual code review system with mailboxes etc., the primary concern has always been "making sure that all legal diffs are legal diffscuss files" - If there's actual interest in using diffscuss as an interchange / export format, then the pragmatic concern of "making sure that all legal diffscuss files are legal diffs" wins

    It's still not at all clear that anyone will actually use diffscuss for interchange / export, but lowering the bar of what would be required for them to do so feels like the pragmatic choice.

    So, we're sold. We'll make the change to use the diff comment syntax.

    点赞 评论 复制链接分享
  • weixin_39853892 weixin_39853892 4月前

    Development will take place in use-patch-comment-syntax.

    点赞 评论 复制链接分享
  • weixin_39853892 weixin_39853892 4月前

    At this point all the tests are passing in the use-patch-comment-syntax branch and quick smoke tests don't turn up problems.

    We'll bake this for a bit by running our own code reviews off this branch and then merge.

    点赞 评论 复制链接分享
  • weixin_39707612 weixin_39707612 4月前

    Thanks , it's great to see such quick progress on this! I am already considering building some tools to interoperate in diffscuss format with our internal code review system, hopefully something good will come out of it!

    I only have one comment: I see that you have dropped the % entirely now and replaced it with a plain #, which means that diffscuss comments can now start with either #* or #-. In my opinion, it would still be good to start diffscuss lines with #% (that is, in practice with #%* and #%-), since: - there is a (very) remote possibility that someone will want to embed markdown in their diff comments, in which case lists would likely start with - or * on commented lines and conflict with diffscuss lines; starting a line with % seems even much less likely to me; - if diffscuss lines start with #%, it leaves you more flexibility to extend the format in the future (e.g. #%+ for a new type of header of something); - for similar reasons, I also think that diffscuss nesting should happen at the % level (i.e. increasing the number of leading %.

    With these changes in mind, the example from the README could look something like:

    
    #%* author: ejorgensen
    #%- I'm a top-level comment.
    #%%* author: bsmith
    #%%- And I'm a reply.
    #%%%* author: sjones
    #%%%- And I'm a reply to the reply.
    #%%* author: jkidd
    #%%- And I'm a second reply to the original top-level comment.
    #%* author: mdarks
    #%- And I'm another top-level comment.
    #%%* author: lbutterman
    #%%- And I'm a reply to the second top-level comment.
    

    I realise none of them is a particularly strong point, but maybe other people can discuss other advantages/disadvantages of the different approaches.

    Thanks, and keep up the great work!

    点赞 评论 复制链接分享
  • weixin_39853892 weixin_39853892 4月前

    Hey ,

    Psyched to hear you're going to try to do some diffscuss integration, and thanks much for the suggestion on the format.

    I'm pretty sure at this point that we'll be keeping the #- / #* syntax, by the following rationale: - having read and written a ton of reviews in diffscuss over the last year, I'm jealous of the extra character--as threads become nested, screen width is at a premium - cutting down the (already very slight) odds that a diff comment will confuse diffscuss doesn't feel worth trading off that precious horizontal space--once we've made the pragmatic choice to open up the possibility that a diff comment can interfere in unlikely circumstances with diffscuss, let's live with that rather than make it a little better at the cost of something else that's valuable.

    Somewhat separately, even once we've decided to stick with # rather than #%, I think it's important to use the * and - as the repeated chars to indicate nesting, again speaking from the experience of reading a bunch of these. This:

    
    #***
    #*** author: someone.com
    #***
    #---
    #--- This is a comment, yay.
    #---
    

    is easier to parse visually than this:

    
    ###*
    ###* author: someone.com
    ###*
    ###-
    ###- This is a comment, yay.
    ###-
    

    That is, the difference between header and body leaps out more clearly, and the lighter weight of the repeated - chars makes it easier to pay attention to the text of the body (compared to the heavier # chars).

    点赞 评论 复制链接分享
  • weixin_39853892 weixin_39853892 4月前

    This has been merged into master and released to pypi.

    Thanks to all who got involved!

    点赞 评论 复制链接分享
  • weixin_39853892 weixin_39853892 4月前

    We'll consider this, but I still incline away for it for a couple reasons, copying from that thread:

    You still get comments which, while unlikely (because they would have to have the second "enabling" character) will confuse a diffscuss parser. I still think allowing for a state of "perfectly legal diff that would have to be altered to allow for diffscuss parsing" is a tough sell...

    Here's another way to think about it: It sounds exciting to think that "all legal diffscuss files are also legal diff files!" In fact, very few diffscuss files will be used as diff files, and those that are can be trivially transformed. It's not awesome on the other hand if "not all legal diff files are legal diffscuss files," which is where piggybacking on the diff comment syntax gets you, because every diffscuss file starts its life as a pure diff file and there's no way to transform an offensive diff file into a legal diffscuss file in this world without data loss.

    点赞 评论 复制链接分享
  • weixin_39791386 weixin_39791386 4月前

    There are very few unified diffs that actually use comments in the first place as 99.99999% of them are autogenerated in a very fire & forget way.

    There are real advantages to using a common format. Namely, an app like github (or pick your clone) could export diffs to only that format, retaining comments. Patch will still work and there would be no need for a separate format. It would certainly help adoption.

    点赞 评论 复制链接分享
  • weixin_39853892 weixin_39853892 4月前

    The "only export to that format" option is compelling, this is true...

    点赞 评论 复制链接分享
  • weixin_39707612 weixin_39707612 4月前

    to echo what said, if diffscuss is "backwards-compatible" with diff, then any code review tool could export in diffscuss format and not risk breaking anything for anyone (unless of course it is already exporting it in a format that overloads comments in the same exact way, but what's the chance of that?).

    点赞 评论 复制链接分享

相关推荐