On 2016-10-11 09:14, Stephen Finucane wrote:
On 2016-10-11 00:51, Russell Currey wrote:

[snip]

- no easy way to download a mbox file of all patches in a series, which would be
very useful

Yup, this is an easy follow up.

- no series in the patch API at all.  This could probably be implemented a few
different ways, but having no series at all is limiting

I think Andrew was working on this so I halted my own efforts here.
Again, this is a separate series though.

[snip]

- some indication that not all patches in the series had been received, i.e.
patch 7/7 arrived but there are only 6 patches

We have this in the admin interface alright, but presenting this in
some real-time manner to the user will require tracking of events. I'd
been hoping to integrate an '/events' API using something like
'django-activity-stream' [1]. This would signal events for things like
"patch has all dependencies met" or "patch state has been changed" via
the API. Once again, though, this seems very much like a separate,
follow-up series.

So I've given the three points above a little more thought and decided it might be reiterating my ideas for how I feel series should function before proceeding.

One of the key differences between how I've approached this and how dlespiau approached it was avoiding his idea of series being "the main object tracked" [1]. I really don't agree with this approach. In my eyes, patches are the one, true thing we should care about and series are simply a way of indicating relationships between said patches [*]. This is why you'll note that there's no new "Series" tab in the UX and it also means that we shouldn't be adding a '/series' endpoint in the API, but rather modifying '/patches' like so:


  GET /patches/?series=1
  [
    "https://example.com/api/v1/patches/1";,
    "https://example.com/api/v1/patches/3";,
    "https://example.com/api/v1/patches/4";
  ]

  GET /patches/4/
  {
    "url": "https://example.com/api/v1/patches/4";,
    ...
    "dependencies": [
      "https://example.com/api/v1/patches/1";,
      null,
      "https://example.com/api/v1/patches/3";
    ],
    "series": "https://example.com/api/v1/patches?series=1";,
    "series_revisions": [
      "https://example.com/api/v1/patches?series=2";
    ]
  }

This means my answers to the above three points are actually subtly different to what I said earlier:

* no easy way to download a mbox file of all patches in a series, which would be very useful

We need to expose a method to download any patch with its dependencies in a mbox file - not the entire series. Something like so?

    GET /patches/4/mbox?include_dependencies=true

This could return an error code if dependencies were missing, I guess? Maybe:

   GET /patches/4/mbox?include_dependencies=true&skip_broken=true

* no series in the patch API at all. This could probably be implemented a few different ways, but having no series at all is limiting

See above. This definitely needs to happen and I'd love help (Daniel?), but it should all happen in '/patches'

* some indication that not all patches in the series had been received, i.e. patch 7/7 arrived but there are only 6 patches

For the dependencies field of '/patch', we should return 'null' for any missing patches (as seen above). If anything is null, series != complete. The '/events' API I previously described would still be helpful to prevent polling though.

I've given this a lot of thought and am more than happy to defend my position, but there's surely stuff I've missed? Whatever the case, let me know what you think and how you would like to proceed here :)

Cheers,
Stephen

[*] There's a few reasons for this, but it ultimately boils down to a series of "-ability" words: "bisectability", "testability", and "backportability", to name a few. Coming from a testing background, I strongly believe in testing each and every patch independently before merging. In addition, I think it should be possible to download patch N or M without having to download N+1 to M at the same time. FWIW, GitHub follow a similar methodology of hiding the patches in favour of exposing a simple "pull request", and this exact model has incurred the wrath of Linus Torvalds at least once [2] :)

[1] http://damien.lespiau.name/2016/02/augmenting-mailing-lists-with-patchwork.html
[2] https://www.wired.com/2012/05/torvalds_github/
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Reply via email to