Skip to content

Mnt rearrange next api again #17265

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 1, 2020
Merged

Mnt rearrange next api again #17265

merged 1 commit into from
Jul 1, 2020

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Apr 29, 2020

PR Summary

The next_api_changes has some recent changes that I think should be reconsidered. Less egregious, the next_api_changes were put into one of four files instead of individual files. As explained in #15158 this was so that the API changes were not a huge mishmash of deprecations, behaviour changes etc, all of which are far easier to track at the outset rather than when the release is made.

However, this leads to endless rebasing, and I don't think is particularly sustainable.

This PR goes back to the one-file-per PR model, but introduces subdirectories:

next_api_changes/
   behaviour/
   deprecations/
   development/
   removal/

so the organization is by subdirectory. The release manager will still have to concatenate the files, but that is pretty easy.

The other change that happened was we renamed the directory 3.3_api_changes which makes no sense if we have a PR that is slated for 3.4 or one that doesn't get into 3.4. If 3.3 gets cut then my API change is a directory that has been removed.

The one-file per PR gets around all these issues because the file will only appear in a tagged release in the right spot.

ping @dstansby @QuLogic @anntzer

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer
Copy link
Contributor

anntzer commented Apr 29, 2020

I haven't had had to arrange the notes for the actual release, and I automated my rebase machinery anyways :-) so I'm fine with anything that works best for the actual release manager.

@jklymak
Copy link
Member Author

jklymak commented Apr 29, 2020

@anntzer right, but you were concerned about misplaced API change notes. I think this satisfies your concern, but I could be wrong....

@anntzer
Copy link
Contributor

anntzer commented Apr 30, 2020

I guess I can just pay more attention to where my notes go :)

QuLogic
QuLogic approved these changes May 1, 2020
View reviewed changes
doc/devel/contributing.rst Outdated
:file:`doc/api/api_changes`, by adding to the relevant file
(see :file:`doc/api/api_changes.rst` for more information)
:file:`doc/api/next_api_changes/behaviour`, by adding a new file with the
naming convention ``20202-02-17-JMK.rst`` where the date is followed by the
Copy link
Member

Choose a reason for hiding this comment

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

That's a bit too far in the future. Also maybe use the name of the template file.

Suggested change
naming convention ``20202-02-17-JMK.rst`` where the date is followed by the
naming convention ``2020-01-01-AL.rst`` where the date is followed by the

doc/devel/contributing.rst Outdated
the most recent :file:`doc/api/api_changes_X.Y`
- Deprecations must be announced via a new file in
the most recent :file:`doc/api/next_api_changes/deprecations/` with
naming convention ``2020-01-01-JMK.rst`` where ``JMK`` are the contributor's
Copy link
Member

Choose a reason for hiding this comment

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

Same name as template:

Suggested change
naming convention ``2020-01-01-JMK.rst`` where ``JMK`` are the contributor's
naming convention ``2020-01-01-AL.rst`` where ``AL`` are the contributor's

@QuLogic
Copy link
Member

QuLogic commented Jun 23, 2020

Oh, this isn't merged yet? No wonder I was confused making the release.

@jklymak
Copy link
Member Author

jklymak commented Jun 23, 2020

Sorry, I think we were waiting for you to do a release and decide if this was the way to go? Now would be a good time to implement if its what is wanted.

@jklymak
Copy link
Member Author

jklymak commented Jun 24, 2020

@anntzer @timhoffm - you guys write the most of these. Would this organization work for you?

@jklymak jklymak requested review from timhoffm and anntzer June 24, 2020 16:05
@anntzer
Copy link
Contributor

anntzer commented Jun 24, 2020

Anything works for me, I wrote my own personal script to automate the rebase ;) so the current approach is good, but putting everything in separate files is good too.

@anntzer anntzer removed their request for review June 24, 2020 16:26
@jklymak jklymak requested a review from dstansby June 29, 2020 17:38
dstansby
dstansby approved these changes Jun 29, 2020
View reviewed changes
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

👍 to having different categories. If individual files makes in-between release work easier also 👍

The release manager will still have to concatenate the files, but that is pretty easy.

Concatenating is easy, tidying up and creating a coherent and consistently structured page from all the snippets less so 😉

@dstansby
Copy link
Member

(obviously the doc build failures should be fixed before this is merged!)

@timhoffm
Copy link
Member

I'm also fine either way.

The advantage of a file with multiple entries is that the author can already place the notes next to similar stuff (at least I've done that). With one entry per file this is exclusively the burden for the release manager. The chronological order is not necessarily a good one.

@jklymak
Copy link
Member Author

jklymak commented Jun 29, 2020

The chronological order is not necessarily a good one.

Thats fair enough. Ideal would be to have some sort of link to the pull request: numpy tracks those somehow https://numpy.org/doc/stable/release/1.19.0-notes.html

@tacaswell tacaswell added this to the v3.4.0 milestone Jun 29, 2020
@jklymak jklymak force-pushed the mnt-rearrange-next-api-again branch from bbd9296 to b84da0c Compare June 29, 2020 20:17
@tacaswell
Copy link
Member

As noted on the call, I think the toc tree glob in https://github.com/matplotlib/matplotlib/blob/master/doc/api/api_changes.rst#L19-L36 needs to also be updated.

@jklymak
Copy link
Member Author

jklymak commented Jun 29, 2020

As noted on the call, I think the toc tree glob in https://github.com/matplotlib/matplotlib/blob/master/doc/api/api_changes.rst#L19-L36 needs to also be updated.

Right, and I need to move what's already been accumulated....

@jklymak jklymak force-pushed the mnt-rearrange-next-api-again branch 2 times, most recently from 7c68201 to 9828374 Compare June 30, 2020 20:04
@jklymak
Copy link
Member Author

jklymak commented Jun 30, 2020

OK, this should properly build now. Sorry about that.

@jklymak jklymak force-pushed the mnt-rearrange-next-api-again branch 2 times, most recently from 0cda28a to b347421 Compare June 30, 2020 23:20
@jklymak
Copy link
Member Author

jklymak commented Jun 30, 2020

This one will actually be a bit of a chore to rebase all the time if we keep getting API changes, so I'll ask for slightly expedited merging if its OK....

QuLogic
QuLogic reviewed Jun 30, 2020
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

The directory is named behavior.

@jklymak jklymak force-pushed the mnt-rearrange-next-api-again branch from b347421 to 73fdc8b Compare July 1, 2020 00:01
@jklymak jklymak force-pushed the mnt-rearrange-next-api-again branch from 73fdc8b to 7c06fcb Compare July 1, 2020 00:03
@QuLogic QuLogic merged commit 9f78b84 into matplotlib:master Jul 1, 2020
@QuLogic
Copy link
Member

QuLogic commented Jul 1, 2020

Travis is reporting twice, and confused.

@jklymak
Copy link
Member Author

jklymak commented Jul 1, 2020

Thanks @QuLogic (and everyone else). Though this PR doesn't preclude our moving to towncrier so far as I'm concerned. I just haven't had time to look at how its set up.

@jklymak jklymak deleted the mnt-rearrange-next-api-again branch July 1, 2020 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@dstansby dstansby dstansby approved these changes

@timhoffm timhoffm Awaiting requested review from timhoffm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jklymak @anntzer @QuLogic @dstansby @timhoffm @tacaswell