Poor Man's MiMa

[japgolly]

Generates API reports so that when we review PRs we can see a list of API changes. This covers the annoying case where a method is moved from one trait to another and it's hard to see which subtypes are affected.

[armanbilge]

😮

[japgolly]

Hehe, yeah I typed that all out manually but it will be automated from here on. Hahaha 😆

Seriously though, once this is merged we should only see small (and clear) changes per PR.

[armanbilge]

Yeah this is amazing!! Great iteration on last nights preview :D

[armanbilge]

Sorry to rain on the parade ... does this support type aliases? I tried commenting out https://github.com/scala-js/scala-js-dom/blob/master/src/main/scala/org/scalajs/dom/package.scala#L7 and the entire API report vanished, except for the header :(

[japgolly]

Hehe, rain away! Better now than post-merge. It does support aliases but it seems the dom package object is missing. It was there before, I'll look into it.

Also the reason it would've come out empty is cos I presume you did an incremental build? That's on my list of things to test and I think you just handed me a sad result.

[armanbilge]

Ahh, oops! It was an incremental build. That must have been it, makes sense. It was working when I changed a return type though 🤔

[japgolly]

@armanbilge Oh crap, something just came up that I need to do over the next few days. Can't believe it's the 11th already. If there's anything that I'm blocking you on just ping me and I'll prioritise that in the little sploshes of free time I get.

[armanbilge]

Ok, I did a clean and re-compile, everything came back :) which unfortunately means no indication of the missing type alias :(

[armanbilge]

Oh, no worries! Take the time you need. I'll go through the list of issues and triage :)

[japgolly]

Thanks! Also I just fixed the package object problem, thanks for catching it so fast

[japgolly]

Apparently package objects aren't considered public. 😕

[armanbilge]

Oh yeah, package objects are weird ... even if they extend a trait, you can't pass them as an instance of that trait :(

[armanbilge]

Yup that seems to have fixed it!! Awesome!

[japgolly]

Hey @armanbilge , so in order for us to enforce that these are up to date, given current limitations this is what I'm thinking:

1. Add an sbt task/alias like prepareForPR that will regenerate the API reports for each valid Scala version, and also ensure that it's a full build (in the event that we can't get this working as expected on incremental builds). Maybe we can even automate committing the reports.
2. Add a PR template (like this) that tells anyone about to raise a PR, "hey make sure you run sbt prepareForPR and check in the reports"
3. CI validates that all API reports are up-to-date and rejects otherwise.

Sound good?

[armanbilge]

👍 a few projects I contribute to have a prePR alias exactly for this sort of thing (also runs formatting, etc.)

[japgolly]

Fantastic! In that case I'll go ahead with this and use prePR as the same name. Thanks!

[armanbilge]

So terrible!! Do we even need to publish for 2.10 other than to be nice?

[japgolly]

Nah just being nice and preserving compat across the 1.x series. Really looking forward to the 2.x branch 😩

[armanbilge]

Looking good to me! What's left?

[armanbilge]

This is gonna be great!! :D

[japgolly]

Looking good to me! What's left?

I wanna test it locally against some of the open PRs to confirm it's gonna work as expected, before we merge. You did a lot of work updating all of those PRs and I don't want us to have to do it again more than once :)

[japgolly]

@armanbilge oh awesome, you're online. This is ready when you are :)

[armanbilge]

Yep, I'm just taking it for a quick spin in #445!

[armanbilge]

Magic!!! Never would have dreamt this up in a million years :)

[japgolly]

Hahaha it's a gaffe-tape solution but it should make our lives easier. Ok cool thanks for the approval, merging! Let's goooooooooooo