Hacker News new | comments | ask | show | jobs | submit login
Show HN: Sedy, a GitHub Bot Bringing Linux Sed to Code Reviews (marmelab.com)
144 points by Kmaschta on Feb 28, 2017 | hide | past | web | favorite | 44 comments



Cool! Looks like a genuinely useful tool, reducing some pretty common friction within the code review process.

Three features I'd want before using it:

1) Rather than triggering Sedy immediately on a reviewer comment, I'd like the trigger to be the original requester reacting to the comment with a thumbs-up. The requester knows what they're trying to say, and they should decide if the changes get made.

2) I wish there were an option to restrict it to comments for supported languages. Your examples are just changing markdown (not code), and I think rightfully so — I can easily see this tool becoming a way for a senior dev reviewer to attempt to avoid the back-and-forth with a junior dev by just posting some complex code substitutions... substitutions which could easily screw things up.

3) sed replacement actually seems too powerful for this job. For instance, if I want to make a replacement like:

    s/**bold** thing/**bold thing**/
...I really don't want to try escaping that without feedback. I'd rather have direct string replacement without regex than the full power of sed. This probably goes against your design goals, but for me it would be much more useful.


Regarding the first point, I think it may be useful to have it need to explicitly be summoned (`@sedy` or whatever), rather than having it try to guess which comments are intended for it. That would also make it simpler to support other bits of sed syntax (like alternate delimiters), that would be hard to auto-recognize as sed commands.


A good thought!

My point was orthogonal, though. I don't want a reviewer directly making committed changes to my code, period. Giving me control isn't about preventing inadvertent calls to sedy. It's about handling when someone suggests a substitution that I don't agree with, or want to reword.

My mindset might be different from what Marmelab intends, but to me, code review is a conversation about the changes I'm responsible for making, not a google doc where anyone can change the content I'm about to push.


I take note of your feedback, thanks!


Also sed supports any character separator ie:

    $ echo http://foobar.com/mai/path/meme.png | sed 's@/mai/path@/sweet/new/path@'
    http://foobar.com/sweet/new/path/meme.png
Consider any separator


It would be great! I have already noted this requested feature on my list.


If you only support substitutions (the "s" command) maybe @sedy <separator><regexp><separator><replacement><separator><flags> that way you could e.g.

    @sedy |foo/bar|foo|g

?


Thank you very much for your feedback.

For third point: it is actually the case! We have not implemented all the features of sed for this reason.

I keep the two first points to my list of requested features, it's very interesting!


Regarding #1, it should be a simple fix once the Reactions API (https://developer.github.com/v3/reactions/) comes out of the preview state.


I share with fiatjaf's sentiment; this is an amazingly cool piece of hackery (that will probably be very useful to many people), but I won't use it.

For those simple types of changes, I like to amend the original commit, rather than make a new one. Of course, I come from using `git send-email` to send patches to a mailing list, where you are expected to send "[PATCH v2]" after you get feedback.


What about a command s/[search]/[replace]/a for "--amend"?


That could win me over! I'm not sure what the appropriate answer is to "who should it put in the committer line?" (especially if author and committer are already different), but that's not a huge problem, I don't think.


It would be a problem for me - suddenly, my local branch and the Github branch are no longer in sync, and I have to untangle this mess before continuing development. (And if I don't notice that Sedy is changing the revision history, and only realize it when I've made a couple of commits and try to push...)


You could do an interactive rebase and make all the Sedy commits fixups.


I like the idea.

Using `s/foo/bar/f` for a `git commit --fixup` of the last commit who modify this change (only if part of the current PR)


"Linux sed"? There's no mention of Linux at https://en.wikipedia.org/wiki/Sed


Ah! I would write Unix but, you know, the marketing ... :p Its not even GNU/Linux !


(sorry, greybeard hat on today)


I forget the exact circumstances, but I was once in a W3C IRC chatroom which had a bot that produced a transcript and obeyed sed commands. It's pretty amazing to be able to type s/their/they're/ and have it actually take effect.


Skype chat does this currently, but only for the last comment, I think.


So does hipchat if i recall right


It does indeed. :)


Whoa.

I will not use this, but I can say this is an amazing piece of hackership.


Thanks!

Any particular reason to not use it? Just to know if we can improve something.


Well, it doesn't solve any particular problem for me, I guess it is because I'm just a hobbyist not managing lots of GitHub repos and pull requests.


In the first screenshot, how does the bot know which instance of 'products' to change? Also /s/themself/themselves for the second one although chances are Sedy can't edit screenshots.


The default rule for now is "change all instance of that word in the whole line".

We'll fine-tune that behavior with the feedback we'll have.


i suggest matching the behavior of slack/skype/hipchat: change only the first instance except in the case of /g (in which case, maybe any instance in the hunk?).


I was kind of hoping for 'we're just trying to make fzaninotto look bad'.


you're killing me


What I do to point out differences is to provide a comment with a code block with diff format.

```diff

-- old text

++ new text

```

Github will highlight that as a diff.


Didn't know that, that's really nice.


I know in bitbucket and others have a nice feature that allows you to edit code inline on the web browser and commit the changes - wouldn't that be a safer approach than editing via commenting? I don't think it would take much more time either.

Maybe doing the reverse, if a change is made through inline edit automatically add a comment with the specific change made.

I just can foresee undesired consequences. Someone not escaping their sed correctly (I.e. they try to replace a string with a slash or apostrophe)


GitHub has that feature too. But it's a lot of context switching to edit the file, commit and return to your code review where you'll need to build the code architecture in your head again.

All of that is explained on the article.


`/s/foo/bar` syntax works in Slack as well!


> Since we need to accept the invitation on behalf of the Sedy bot, you’ll have to notify us about it. For the time being, we only accept notifications by postcard

Huh?


We'll switch to Github Integration soon to avoid this issue.


How much of the power of sed does it bring?

I feel like other sed expressions might be even more useful in this format. For example:

    200i #TODO optimize this
will insert a comment before line 200 and:

    s/.*goto.*/cowsay \0/e
    T
    s%^%//%
    s%\n%\0//%g
will take every line that contains a goto and replace it with a commented out cowsay version of itself.


Does s:/bin:/usr/bin:g work?


The global flag is planned to edit an entire file. And be able to change the separator could be a very nice feature! Thanks for the idea.

The project is still young and we'll implement in priority what save time to developers.


You might also consider the "i" case insensitivity flag.


It's in the pipeline!


This is a nice implementation of sed, but what if you replace something you did not intend to?


Too bad!




Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | Legal | Apply to YC | Contact

Search: