Skip to content

DOC Fix "Duplicate labels" & "Duplicate .. target" sphinx warn#14931

Merged
rth merged 6 commits intoscikit-learn:masterfrom
cmarmo:duplicate_labels
Sep 11, 2019
Merged

DOC Fix "Duplicate labels" & "Duplicate .. target" sphinx warn#14931
rth merged 6 commits intoscikit-learn:masterfrom
cmarmo:duplicate_labels

Conversation

@cmarmo
Copy link
Copy Markdown
Contributor

@cmarmo cmarmo commented Sep 9, 2019

Reference Issues/PRs

Fixes #11533

What does this implement/fix? Explain your changes.

  1. duplicate label coding-guidelines fixed removing the section coding-guidelines from contributing.rst (this was a duplicate of the same section in develop.rst)

  2. duplicate label in "what's new" sections have been fixed following "WARNING: duplicate label" because of include sphinx-doc/sphinx#1668

  3. In addition to that
    duplicate explicit target name warnings are fixed following Duplicate explicit target name errors sphinx-doc/sphinx#3921

Any other comments?

  1. Ask for manually rename the two files included in whats_new.rst, maybe a symbolic link to (e.g.) last.inc and previous.inc could be also be a solution? ... but then you have to exclude some .rst from the build... Anyway this is a fix..

Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

It seems that the duplication of the coding-guidelines section was introduced in #14611 when addressing #14582. So indeed keeping the one in develop.rst and deleting the duplicate from contributing.rst seems to be the right change.

gradient boosting trees, namely :class:`HistGradientBoostingClassifier`
and :class:`HistGradientBoostingRegressor`, inspired by
`LightGBM <https://github.com/Microsoft/LightGBM>`_.
`LightGBM <https://github.com/Microsoft/LightGBM>`__.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did not know double underscores were needed for inline links. Is this a new-ish syntax or has it always been this way? Anyway the links seem to work in the Circle CI rendered HTML.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just read the discussion in sphinx-doc/sphinx#3921. Sorry for the noise.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Sep 11, 2019

Thanks @cmarmo for syncing master into this branch to resolve the conflicts. However conflicts will likely happen again if we do not merge this PR quickly.

@jnothman what's you opinion on using the .inc suffix for the whatsnew files?

Copy link
Copy Markdown
Member

@rth rth 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!

LGTM as well aside from renaming .rst to .inc. I was not sure why we need to do that? Local text editors (or Github for that matter) will no longer recognise it as rst, so I think that is not ideal.

@cmarmo
Copy link
Copy Markdown
Contributor Author

cmarmo commented Sep 11, 2019

LGTM as well aside from renaming .rst to .inc. I was not sure why we need to do that? Local text editors (or Github for that matter) will no longer recognise it as rst, so I think that is not ideal.

@rth , I agree, but following sphinx-doc/sphinx#1668 I have to find a way to exclude those two files from the build then... this is one possible solution.

Copy link
Copy Markdown
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.

Okay by me except for merge conflicts created in open pull requests

@rth
Copy link
Copy Markdown
Member

rth commented Sep 11, 2019

aside from renaming .rst to .inc.

I have to find a way to exclude those two files from the build then... this is one possible solution.

Or we can just accept that this warning will remain. I don't have a strong option about it. We can also merge as is.

@cmarmo
Copy link
Copy Markdown
Contributor Author

cmarmo commented Sep 11, 2019

I have to find a way to exclude those two files from the build then... this is one possible solution.

Or we can just accept that this warning will remain. I don't have a strong option about it. We can also merge as is.

Well ... if you ask me, I'd rather accept the warning. Changing the name of those specific two files will introduce manual maintenance each time you produce a new release.

@rth
Copy link
Copy Markdown
Member

rth commented Sep 11, 2019

Well ... if you ask me, I'd rather accept the warning.

Let's do it then.

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Sep 11, 2019 via email

@cmarmo
Copy link
Copy Markdown
Contributor Author

cmarmo commented Sep 11, 2019

Well ... if you ask me, I'd rather accept the warning.

Let's do it then.

Done :-)

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Minor comment otherwise LGTM.

Test fails because master is broken...

Copy link
Copy Markdown
Member

@rth rth 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 !

@rth rth changed the title Fix "Duplicate labels" and "Duplicate explicit target" sphinx Warnings DOC Fix "Duplicate labels" & "Duplicate .. target" sphinx warn Sep 11, 2019
@rth rth merged commit f24a501 into scikit-learn:master Sep 11, 2019
@cmarmo cmarmo deleted the duplicate_labels branch September 11, 2019 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate Label warnings in sphinx

4 participants