Skip to content

Change the way API changes are documented #15158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 2, 2019

Conversation

dstansby
Copy link
Member

Given the pace at which we are making API changes in each new release, the current method of one change/file, then merge just before a release isn't very sustainable. I did the 3.1.0 changes, and I've given up doing the 3.2.0 changes because it's a very boring task. Problems are:

  • The merger has to decide how to categorise each change, despite not being involved in most of them
  • Each note is written in a different style, which in places has to be fixed
  • It takes a really long time to merge all the individual files
  • It is very easy for individual API change files to get accidentally lost in the merge up to a big file

So I am proposing a change such that all API changes are added at the time the change is made to one of four files (categories up for discussion here):

  • Behaviour changes
  • Deprecations
  • Removals
  • Development changes

I know this will involve merge conflicts along the way, but I honestly think it's a hit worth taking to make the ultimate task of putting together a coherent API changes page easier.

@dstansby dstansby added this to the v3.3.0 milestone Aug 30, 2019
@anntzer
Copy link
Contributor

anntzer commented Aug 30, 2019

What about #14589 (the towncrier PR)? It looks like the necessary upstream PR (twisted/towncrier#157) already went in.
Also, given that I'm not the one doing the merging but probably one of those creating the most work there, I'm happy with whatever solution you prefer, including towncrier, even if this requires some changes on my workflow...

@NelleV
Copy link
Member

NelleV commented Sep 3, 2019

@dstanby I think that your proposed workflow would work fine.
Out of curiosity, how much changes do we have per releases and how are they distributed between the four categories you are proposing?

In terms of writing style, that should be fixed prior to merging the PR. Might be worth adding a note to the reviewing guidelines about that.

@dstansby
Copy link
Member Author

dstansby commented Sep 3, 2019

@anntzer even though it's merged, it's not released and I don't think towncrier is the most actively developed project.

I think (w/ no real evidence) adding to the same file that already has some changes will help first-timers see what kind of thing to write, and will hopefully reduce the amount of copy editing that needs to be done at release time.

@dstansby
Copy link
Member Author

dstansby commented Sep 3, 2019

@NelleV Most stuff by quantity is deprecations, but excluding @anntzer's commits(*) I'd say it's fairly well split between the 4 categories.

(*) Not a complaint at all, but he seems to do most of the deprecating ;)

@tacaswell
Copy link
Member

I know this will involve merge conflicts along the way,

We used to do it that way and it caused so many re-bases that we moved to the many files.

@NelleV
Copy link
Member

NelleV commented Sep 3, 2019

Could we do a mixture of both? Split between four categories, but keep several files with a strong naming conventions and structure, and "simply" do a cat before the release?

@dstansby dstansby closed this Sep 5, 2019
@dstansby dstansby deleted the apichanges branch September 5, 2019 17:48
@jklymak
Copy link
Member

jklymak commented Sep 5, 2019

We discussed this on the weekly call, and there was support for exactly the approach in this PR. Yes, there will be some rebasing needed, but usually new developers who would be burdened by rebasing are not doing deprecations etc which get hit heavily. And splitting into four categories lessens the chance of conflicts.

@dstansby
Copy link
Member Author

dstansby commented Sep 5, 2019

Woops, not sure why I closed this!

@dstansby dstansby restored the apichanges branch September 5, 2019 20:13
@dstansby dstansby reopened this Sep 5, 2019
@tacaswell
Copy link
Member

@dstansby What is the state of this? I think we want to get this enabled for 3.3 ASAP!

@dstansby
Copy link
Member Author

Thanks for the nudge, I think modulo getting the docs to build this is almost there. Will try and sort this out in the next couple of days!

@dstansby dstansby force-pushed the apichanges branch 2 times, most recently from a4456eb to 2228c36 Compare September 25, 2019 16:52
@dstansby dstansby marked this pull request as ready for review September 27, 2019 10:40
@dstansby
Copy link
Member Author

I think this is ready now!

@timhoffm
Copy link
Member

Doc builds complain:

/home/circleci/project/doc/api/api_changes.rst:40: WARNING: Problems with "include" directive path:
InputError: [Errno 2] No such file or directory: 'api/api_changes/api_changes_3.2.0.rst'.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will need follow up PRs from folks who have already committed API changes?

@timhoffm
Copy link
Member

timhoffm commented Oct 2, 2019

There's four API change notes on master so far. I can handle these in one go.

Open PRs will need an update by the respective author.

@jklymak jklymak merged commit 8448703 into matplotlib:master Oct 2, 2019
@dstansby dstansby deleted the apichanges branch October 2, 2019 21:42
@jklymak jklymak mentioned this pull request Apr 29, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants