8000 Suggestion for whats_new warning solution. by cmarmo · Pull Request #15288 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Suggestion for whats_new warning solution. #15288

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 10 commits into from
Nov 5, 2019
Merged

Suggestion for whats_new warning solution. #15288

merged 10 commits into from
Nov 5, 2019

Conversation

cmarmo
Copy link
Contributor
@cmarmo cmarmo commented Oct 18, 2019

Reference Issues/PRs

This pull request suggests a solution for sphinx warnings generated by double inclusion of some what's new files (See #11533).

What does this implement/fix? Explain your changes.

What's new for latest releases are no more included in whats_new.rst but added in a toctree (maxdepth 3).

Any other comments?

PROs:

CONs:

  • labels of changelog type are not visible in whats_new.html
  • others?

@thomasjpfan
Copy link
Member

Can the labels be placed in its own file and then included at the top of the whats_new files?

@jnothman
Copy link
Member

I'm okay to link rather than include.

@cmarmo
Copy link
Contributor Author
cmarmo commented Oct 21, 2019

Can the labels be placed in its own file and then included at the top of the whats_new files?

Sure... I think my sentence was unclear. What I meant is that the actual version allows you to visualize labelled modifications of the APIs, my suggestion doesn't. The legend is displayed anyway.

@jnothman
Copy link
Member
jnothman commented Oct 22, 2019 via email

@cmarmo
Copy link
Contributor Author
cmarmo commented Oct 22, 2019

I'm still not understanding in what way the proposal is deficient?

Well, that's perfect! Maybe you can approve it then! :)

jnothman
jnothman reviewed Oct 22, 2019
View reviewed changes
doc/whats_new.rst Outdated
Latest Releases
=================
.. toctree::
:maxdepth: 3
Copy link
Member

Choose a reason for hiding this comment

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

Why this choice of maxdepth? I'd consider less, or even to just make this part of the same listing as below.

8000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to keep more details about recent releases wrt the older ones, and stay close to the actual version in some way. maxdepth:3 gives you the list of the modules affected by modifications.
If this is usefulness I will change it.

@thomasjpfan
Copy link
Member

I was considering something like this: cmarmo#3

@jnothman
Copy link
Member
jnothman commented Oct 22, 2019 via email

@cmarmo
Copy link
Contributor Author
cmarmo commented Oct 23, 2019

I was considering something like this: cmarmo#3

Thanks @thomasjpfan, and sorry ... for some reason I didn't realize that you actually asked for this change.

@cmarmo
Copy link
Contributor Author
cmarmo commented Oct 23, 2019

I was considering something like this: cmarmo#3

Thanks @thomasjpfan, and sorry ... for some reason I didn't realize that you actually asked for this change.

But, some new sphinx warnings are generated by cmarmo#3... :(
I'm working on that.

jnothman
jnothman reviewed Oct 30, 2019
View reviewed changes
Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I think it is fine to keep each release change log separate. I think this improves web site maintainability. I don't think we need a "Latest releases" heading

@cmarmo
Copy link
Contributor Author
cmarmo commented Nov 1, 2019

All releases are now at the same level. I have also removed the big_toc_css: I did not like the big titles in a simple list. Let me know if you really prefer the old rendering....

@cmarmo
Copy link
Contributor Author
cmarmo commented Nov 1, 2019

Note that the sphinx warning has been added by b92455a#diff-981322da7c3c606f7c65889a9c77eda9

jnothman
jnothman approved these changes Nov 2, 2019
View reviewed changes
Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Otherwise lgtm

doc/changelog_legend.inc Outdated
@@ -0,0 +1,12 @@
Legend for changelogs
Copy link
Member

Choose a reason for hiding this comment

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

Why not put this file in the whats_new folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed... definitely cleaner, thanks.

adrinjalali
adrinjalali approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Thanks @cmarmo

@adrinjalali adrinjalali merged commit 9e08b14 into scikit-learn:master Nov 5, 2019
@cmarmo cmarmo deleted the whatsnewwarnings branch November 5, 2019 17:47
@thomasjpfan thomasjpfan mentioned this pull request Nov 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cmarmo @thomasjpfan @jnothman @adrinjalali
0