Why didn’t I respond to your pull request?

I have some fairly popular open source packages up on GitHub. Happily, I get people submitting pull requests, adding features or fixing bugs. It’s great when this happens, because people are doing work that I don’t want to do / haven’t gotten to yet / didn’t think of.

…but I’m pretty bad at responding to these. They tend to languish for a while before I get to them. There’s a decent number which I’ve never even replied to.

Why is this?

Fundamentally, it’s because reviewing a pull request is potentially a lot of work… and the amount of work isn’t necessarily obvious up-front. This means I only tend to do reviews for anything which isn’t obviously trivial when I’m feeling energetic and like I have a decent amount of free time.

First, there’s some common potential problems which might turn up:

  1. It does something I don’t want to include in the project. This is the only outright deal-breaker. Project owner’s prerogative.

  2. It doesn’t work. This happens more often than you’d think, generally because the submitter has written code for the exact use-case they had, and hasn’t considered what will happen if someone tries to use it in a different way.

  3. It works, but not in the way I want it to. For instance, it might behave inconsistently with existing features, and I’d want it adjusted to match.

  4. It should be written differently. This tends to include feedback like “you should use this module” / “this code should really go over here” / “this duplicates code”.

  5. It has coding style violations. Things like indentation, variable names, or trailing whitespace. These aren’t functional problems, but I still don’t want to merge them, because I’d just have to make another commit to fix them myself.

Once I’ve read the patch and given this feedback, which might itself take a while since design feedback and proper testing that exercises all code paths isn’t necessarily quick, I’ll respond asking for changes. Then there’s an unknown wait period while the submitted finds time to respond to those changes. Best-case for me, they agree with everything I said, make all requested changes perfectly, and update their pull request with them! Alas, people don’t always think I’m a font of genius, so there’s an unknowable amount of back-and-forth needed to find a compromise position we both agree on. This generally involves enough time between responses that the specifics of the patch aren’t in my head any more, so I have to repeat the review process each time.

What can I do better?

One obvious fix: delegate more. Accept more people onto projects and give them commit access, so I don’t have to be the bottleneck. I’m bad at doing this, because my projects tend to start as “scratch my itch” tasks, and I worry about them drifting away from code I’m personally happy with. Plus, I feel that if the problem is “I don’t review patches promptly”, “make someone else do it instead” is perhaps disingenuous as a response. 😀

So, low-hanging fruit…

Coding style violations, despite being trivial, are probably the most common sources of a patch sitting unmerged as I wait for someone to respond to a request to fix them. This is kind of my fault, because I have a bad habit of not documenting the coding style I expect to be used in my projects, relying on people writing consistent code by osmosis. Demonstrably, this doesn’t work.

As such, I’m starting to add continuous integration solutions like Travis to my projects. Without any particular work on my part, this lets me automatically warn contributors about coding style concerns which can be linted for, via tools like flake8 or editorconfig. If their editing environment is set up for it, they’ll get feedback as they write their patch… and if not, they’ll be told on GitHub when a pull request fails the tests, and don’t have to wait for me to get back to them about it.

Build Status

The “it doesn’t work” issue can be worked into this framework as well, with a greater commitment to writing tests on my part. If my project is already well-covered, I can have the CI build check test coverage, and thus require that contributors are providing tests that cover at least most of what they’re submitting, and don’t break existing functionality.

This should reduce me to having to personally respond to a smaller set of “how should this be written?” issues, which I think will help.

Sublime Text 2 git plugin

I wrote a git plugin for Sublime Text 2.

I’d decided to try Sublime out for work to see how it compared to TextMate… and thus some degree of git integration was required. Given that it’s been out since January, I was surprised that there wasn’t already a solid git plugin.

I did find this one, admittedly, but I decided that I didn’t like how it fit in with Sublime. It’s built around menus and keybinds, whereas I felt that setting everything up as commands in the palette and hooking as much stuff as I could into the fuzzy search was the way to go.

Working on the plugin was a good exercise in getting me used to Sublime. I’m fairly sold on it as a result. It’s philosophically somewhat similar to TextMate, but with some of TextMate’s rough edges smoothed out.

(Short rant: if the recently announced TextMate2 alpha doesn’t get rid of the single-character undo buffer… I don’t know what I’ll do. It’s certainly the biggest single complaint I have about TextMate nowadays.)

A shadow is upon us

Other people contributing to my projects is often the cause of my improving them. This is because people tend to contribute something that works for the case they care about, without necessarily testing how it combines with the rest of the product. There’s nothing wrong with this. They did some work and wanted to give it back; that’s how open source should work.

A case in point here is how shadows just got added to maphilight. A pull request was submitted for a commit that added shadow support for rectangles in canvas only. I accepted the request because, hey, that’s a nice improvement, and it seemed to work. But I wasn’t really happy with rectangles-only.

So, I started fiddling with it. I had, for whatever reason, never touched shadows in canvas before. In fact, it’s been quite a while since I did the research into canvas that was involved in writing maphilight in the first place.

Looking at the commit I see that the shadows have been implemented with a combination of clipping regions and redrawing the shape with some shadow options on the path.

Now, I’m confused by the clipping being done, since I’ve never seen it work quite like that before. It’s drawing a rectangle around the whole canvas, then another around the rectangle we’re shadowing, and telling it to clip. So I do a bit of testing, and I find that this isn’t doing what I think the submitter meant it to.

I think they wanted it to set up a clip region only outside the shape, so that the shadows they drew wouldn’t appear inside it. However, in practice it seems that it’s just adding the two rectangles together and leaving us with a clip region the size of the whole canvas. It’s possible that this did work in another browser, but not in Chrome where I was testing…

Since subtractive clipping obviously wasn’t the answer, I looked into globalCompositeOperation to clean up after the fill. It turned out that destination-out was the operation I needed to empty my shape. Also, because the shadow-drawing had been added after the regular shape, I had to move it to be before that, otherwise the shadow was being drawn on top of the stroke and cleaning it up would wipe out the fill.

Okay! Now we have outline shadows.

But, another issue with this method: it’d fill and stroke on the shape, regardless of the settings you were using. If you had no fill / stroke it’d use the default (flat black) settings, which are ugly. Also, it harmed your opacity settings — the stroke and fill were being done twice. So when I added a shadow to a strokeless mostly-transparent rectangle I noticed that it gained a thin black outline, and was darker than it should have been.

I messed around a little bit with trying to erase the fill or stroke, but eventually decided that this was more hassle than it was worth. What wound up being the simplest option was drawing the shape massively off the edge of the canvas, and using shadow offsets to cast the shadow into the right spot.

Now we had a shadow that didn’t involve drawing anything stroking or filling onto the canvas near our existing shape. At this point I had completely rewritten the code I’d merged in. About the only thing remaining was the option names Raven24 had chosen.

I went ahead and added some options for whether the shadow was cast inside or outside the shape, since I could see reasons for both, and added some overrides for whether it’d be casting from the fill or the stroke, since the varying possible configurations made it difficult to reliably guess which would look better.

I still didn’t add it to non-canvas. Largely because now that IE has finally given in and implemented canvas I view that as being a dead branch. Needs to keep working, and any major changes have to be ported over… but minor display differences are somewhat acceptable. Also, I don’t have access to IE right now, since I’m away from home. If I ever have reason to look into shadows in VML I’m sure I’ll add it in then, for the heck of it.

You can see all this in action on the demo page if you’re interested.

And that’s how community involvement improves things. 😀