Skip to content

MNT: fix api changes link in PR template #27641

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
Jan 15, 2024

Conversation

story645
Copy link
Member

PR summary

As @QuLogic pointed out in #27310 (comment), the pr template now links to the wrong section so changed that. Also tried making the "go to API changes doc" clearer , but don't love the use of admonitions here.

PR checklist

@story645 story645 mentioned this pull request Jan 11, 2024
5 tasks
@QuLogic
Copy link
Member

QuLogic commented Jan 11, 2024

I believe this should have been labelled with Documentation: devdocs but wasn't due to #27642.

@story645
Copy link
Member Author

Yeah I didn't label this just to see if the action worked!

@QuLogic
Copy link
Member

QuLogic commented Jan 12, 2024

Tried to re-run it, but it's still complaining about the same stuff, so I guess it's pulling from the base commit, and not top of main. Maybe if you rebase this one it'll figure itself out?

@story645
Copy link
Member Author

so I guess it's pulling from the base commit

is that how it ended up w/ the toolkit label?

@QuLogic
Copy link
Member

QuLogic commented Jan 13, 2024

Looks like that one is another minor issue in the config: #27644

@QuLogic QuLogic added this to the v3.9.0 milestone Jan 13, 2024
@story645
Copy link
Member Author

@dstansby @QuLogic do either of you have a preference for admonition vs card for the API changes call out?

@timhoffm
Copy link
Member

@dstansby @QuLogic do either of you have a preference for admonition vs card for the API changes call out?

To me, card feels a bit out of place. Either we regard "API changes" a proper part of the coding guidelines, in which case we should keep the original heading (optionally move that section further down). Or we regard it as a cross-ref hint (think: "if you contribute code, please consider our API changes policy as well"), in which case, I'd go with .. note::.

@story645
Copy link
Member Author

To me, card feels a bit out of place. Either we regard "API changes" a proper part of the coding guidelines, in which case we should keep the original heading (optionally move that section further down). Or we regard it as a cross-ref hint (think: "if you contribute code, please consider our API changes policy as well"), in which case, I'd go with .. note::.

So this change was motivated by the link out to the API guidelines getting kinda buried:

image

I tried a card and I did not like it. Trying a highlight approach now, will post back.

@github-actions github-actions bot removed the Documentation: devdocs files in doc/devel label Jan 14, 2024
@story645 story645 changed the title DOC: fix api changes link in PR template and make API changes linkout more prominant in coding guide DOC: fix api changes link in PR template Jan 14, 2024
@story645
Copy link
Member Author

Changed this PR to just change the link on the PR template so it can go in quickly, moving the docs changes to a different PR

@story645 story645 changed the title DOC: fix api changes link in PR template MNT: fix api changes link in PR template Jan 14, 2024
@timhoffm timhoffm merged commit 59f6e2b into matplotlib:main Jan 15, 2024
@story645 story645 deleted the pr-template branch January 15, 2024 01:54
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.

4 participants