4 ms·
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) Rat
by unholiness 10y ago
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.
- Kmaschta 10y agoThank 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!
- LukeShu 10y agoRegarding 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.
- Kmaschta 10y agoI take note of your feedback, thanks!
- masklinn 10y agoIf 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 ?
- SEJeff 10y agoAlso 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
- Kmaschta 10y agoIt would be great! I have already noted this requested feature on my list.
- unholiness 10y agoA 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.
- citrusui 10y agoRegarding #1, it should be a simple fix once the Reactions API (https://developer.github.com/v3/reactions/ https://developer.github.com/v3/reactions/) comes out of the preview state.