Skip to content
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

Allow to bail out extlink replacement suggestion #10112

Closed
gaborbernat opened this issue Jan 17, 2022 · 31 comments · Fixed by #10137
Closed

Allow to bail out extlink replacement suggestion #10112

gaborbernat opened this issue Jan 17, 2022 · 31 comments · Fixed by #10137
Labels
extensions type:enhancement enhance or introduce a new feature
Milestone

Comments

@gaborbernat
Copy link
Contributor

gaborbernat commented Jan 17, 2022

Feature added via #9800. Consider the following ext link:

# conf.py
extlinks = {
    "user": ("https://github.com/%s", "@"),
}

and the following text:

All pull requests and merges to the ``main`` branch are tested using `GitHub Actions <https://github.com/features/actions>`_ .
hardcoded link 'https://github.com/features/actions' could be replaced by an extlink (try using ':user:`features/actions`' instead)

Can we somehow bailout out the check here, or perhaps the suggestion should only apply if there's no / in the extlink, @tk0miya what do you think? cc @hoefling

@gaborbernat gaborbernat added the type:enhancement enhance or introduce a new feature label Jan 17, 2022
@ssbarnea
Copy link
Contributor

ssbarnea commented Jan 17, 2022

This affected me too on other project and what I find it quite problematic for two reasons:

  • it seems that this feature does not work on macos, I get this warning only on linux --- weird
  • i do not see any option to silence it and because I use strict mode, it broke the CI
  • people may want to avoid using sphinx specific constructs in order to keep the file editable and rendable by more tools

The only workaround that I see now is to pin-down sphinx to <4.4.0.

@tk0miya
Copy link
Member

tk0miya commented Jan 17, 2022

I agree it's needed to control the check feature (#10113 is a related story). I'm not sure it's really needed to control the check by file or individual URL. So I'd like to add an option to enable/disable the checks for the whole of the project.

@gaborbernat
Copy link
Contributor Author

I'm not sure it's really needed to control the check by file or individual URL.

I for one like this feature for all my sphinx only files, but then there's an index file used by pypi.org where a file-level exclude list would be helpful. The individual URL disable would be there to bypass bugs of detection without needing to turn off the feature entirely.

@tk0miya
Copy link
Member

tk0miya commented Jan 17, 2022

it seems that this feature does not work on macos, I get this warning only on linux --- weird

IMO, this feature is not related to the OS. So I'm not sure why it does not work on macOS. How about call make clean before building?

people may want to avoid using sphinx specific constructs in order to keep the file editable and rendable by more tools

Understandable. The PyPI's case is one of them.

@tk0miya
Copy link
Member

tk0miya commented Jan 17, 2022

The individual URL disable would be there to bypass bugs of detection without needing to turn off the feature entirely.

Please let me know what case do you want to disable the check? I can understand the PyPI's case. But another one is not yet.

@tk0miya tk0miya added this to the 4.4.1 milestone Jan 17, 2022
@gaborbernat
Copy link
Contributor Author

Please let me know what case do you want to disable the check? I can understand the PyPI's case. But another one is not yet.

See my first post here #10112 (comment) with the github actions link being suggested as a user link.

@hoefling
Copy link
Contributor

I'm actually fine with reverting #9800 completely. While issuing the warnings actually makes sense for every hardcoded link that can be replaced with intersphinx (as suggested in #9626), it's just not worth it with extlinks since it now forces to use the roles even for unrelated URLs. In the example listed by @gaborbernat, it would mean

extlinks = {
    "user": ("https://github.com/%s", "@"),
    "feature": ("https://github.com/%s", "feat"),
}

and rewriting the link to :feat:`GitHub Actions <actions>`, and it's just too much fuzz for a single link.

@gaborbernat
Copy link
Contributor Author

Hence why I was proposing that the suggestion should only apply if there's no / in the part represented by %s. I think that'd fix 99% of the cases here.

ansible-zuul bot pushed a commit to ansible/ansible-navigator that referenced this issue Jan 19, 2022
Cap the version `Sphinx` below 4.4.0

The latest version of sphinx was causing the following errors, and therefore is being capped to version prior to the version causing the errors.
/home/docs/checkouts/readthedocs.org/user_builds/ansible-navigator/checkouts/774/docs/changelog.md:16: WARNING: hardcoded link 'https://github.com/twisted/towncrier' could be replaced by an extlink (try using ':gh:`twisted/towncrier`' instead)
/home/docs/checkouts/readthedocs.org/user_builds/ansible-navigator/checkouts/774/docs/installation.md:3: WARNING: hardcoded link 'containers/podman#8016' could be replaced by an extlink (try using ':gh:`containers/podman/issues/8016`' instead)

Detailed information about the changes introduced in sphinx causing the errors can be found here:
Related: sphinx-doc/sphinx#10112
The following issue was created to revert this PR once the sphinx error is fixed or can be avoided:
See #776

Reviewed-by: Bradley A. Thornton <bthornto@redhat.com>
Reviewed-by: None <None>
Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
cidrblock pushed a commit to cidrblock/ansible-navigator that referenced this issue Jan 19, 2022
Cap the version `Sphinx` below 4.4.0

The latest version of sphinx was causing the following errors, and therefore is being capped to version prior to the version causing the errors.
/home/docs/checkouts/readthedocs.org/user_builds/ansible-navigator/checkouts/774/docs/changelog.md:16: WARNING: hardcoded link 'https://github.com/twisted/towncrier' could be replaced by an extlink (try using ':gh:`twisted/towncrier`' instead)
/home/docs/checkouts/readthedocs.org/user_builds/ansible-navigator/checkouts/774/docs/installation.md:3: WARNING: hardcoded link 'containers/podman#8016' could be replaced by an extlink (try using ':gh:`containers/podman/issues/8016`' instead)

Detailed information about the changes introduced in sphinx causing the errors can be found here:
Related: sphinx-doc/sphinx#10112
The following issue was created to revert this PR once the sphinx error is fixed or can be avoided:
See ansible#776

Reviewed-by: Bradley A. Thornton <bthornto@redhat.com>
Reviewed-by: None <None>
Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
cidrblock pushed a commit to cidrblock/ansible-navigator that referenced this issue Jan 20, 2022
Cap the version `Sphinx` below 4.4.0

The latest version of sphinx was causing the following errors, and therefore is being capped to version prior to the version causing the errors.
/home/docs/checkouts/readthedocs.org/user_builds/ansible-navigator/checkouts/774/docs/changelog.md:16: WARNING: hardcoded link 'https://github.com/twisted/towncrier' could be replaced by an extlink (try using ':gh:`twisted/towncrier`' instead)
/home/docs/checkouts/readthedocs.org/user_builds/ansible-navigator/checkouts/774/docs/installation.md:3: WARNING: hardcoded link 'containers/podman#8016' could be replaced by an extlink (try using ':gh:`containers/podman/issues/8016`' instead)

Detailed information about the changes introduced in sphinx causing the errors can be found here:
Related: sphinx-doc/sphinx#10112
The following issue was created to revert this PR once the sphinx error is fixed or can be avoided:
See ansible#776

Reviewed-by: Bradley A. Thornton <bthornto@redhat.com>
Reviewed-by: None <None>
Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
sampsyo added a commit to beetbox/beets that referenced this issue Jan 20, 2022
A recent change in Sphinx introduced a new warning about missed extlink
opportunities:
sphinx-doc/sphinx#10112

These warnings are causing spurious CI failures. Some of these
suggestions are good but many of them are not, and there is not
currently a way to disable the warning (globally or locally). So the
only workable solution currently seems to be to pin an old version of
Sphinx in CI for now. Hopefully there will be an option to disable this
in 4.4.1, at which point we can unpin.
tk0miya added a commit to tk0miya/sphinx that referenced this issue Jan 22, 2022
… default

This disables hardcoded links detector by default.  Instead, a new
configuration `extlinks_detect_hardcoded_links` to enable it explicitly.
tk0miya added a commit to tk0miya/sphinx that referenced this issue Jan 22, 2022
… default

The hardcoded links detector added since 4.4.0 causes troubles in many
projects.  Therefore, this disables it by default, and adds a new
configuration `extlinks_detect_hardcoded_links` to enable it explicitly.
@tk0miya
Copy link
Member

tk0miya commented Jan 22, 2022

I think extlinks can accept shortcuts contains /. For example, :repo:`sphinx-doc/sphinx` should be allowed. So -1 for the rule.

@gaborbernat
Copy link
Contributor Author

Be that so, but I don't think your commit solves this issue. Adding a global disable flag is not what this issue is about. I purposefully formulated it to keep the feature but allow disabling it where the check makes invalid suggestions 🤔

@tk0miya
Copy link
Member

tk0miya commented Jan 23, 2022

Indeed. My PR and your trouble are different topics. So the title of the PR is not good.

@tk0miya tk0miya modified the milestones: 4.4.1, 4.5.0 Jan 23, 2022
@acolomb
Copy link

acolomb commented Jan 24, 2022

I think extlinks can accept shortcuts contains /. For example, :repo:`sphinx-doc/sphinx` should be allowed. So -1 for the rule.

I'd also like to keep the possibility of using / in the replacement string. Example: :pull:`1234/files` for directly linking to a PR's diff view. Noticed this while working on https://github.com/syncthing/docs. Glad to see activity on a quick fix in #10126.

@tk0miya
Copy link
Member

tk0miya commented Jan 30, 2022

IMO, our recommendation logic does not match to the URL structure of GitHub. But extlink is not only for GitHub. So I'd not like to add a special logic for the specific site. If you don't prefer the exclusion list, all I can say is "please disable the feature via #10126.

@asottile
Copy link
Contributor

I don't think github is special here -- the same could be said for any other vcs, or really any other popular website. I do think that the heuristic of "contains a slash" is a pretty good approximation for disabling the warning (that would solve 99% of the false positive warnings)

@mzjn
Copy link

mzjn commented Jan 30, 2022

Here is a workaround. Add the following lines to conf.py to suppress the warnings:

from sphinx.util import logging

linklogger = logging.getLogger('sphinx.ext.extlinks')
linklogger.setLevel(40) # Ignore messages less severe than ERROR

@tk0miya
Copy link
Member

tk0miya commented Jan 30, 2022

I don't think github is special here -- the same could be said for any other vcs, or really any other popular website.

Are you saying all case of extlink usage does not contain a slash or it's a special case? If so, we should mention the restrictions on the document of extlink. But I'm not sure the rule is a standard rule...

@asottile
Copy link
Contributor

no, you're not understanding me or gaborbernat

we are merely suggesting that the WARNING should not fire if the replacement contains a slash

that would eliminate the frustration with the new warning which has a very high false positive rate in its current implementation

we are not talking about disallowing slashes in extlinks or changing how extlinks work at all -- merely the warning

#10137 implements that and contains a test demonstrating this behaviour

@tk0miya
Copy link
Member

tk0miya commented Jan 30, 2022

Really? The recommendation logic you proposed will never warn for references containing a slash. It means it does not consider URLs containing a slash are not candidates of extlink, right?

that would eliminate the frustration with the new warning which has a very high false positive rate in its current implementation

I understand the current implementation is very noisy for some people. So I thought disable it by default via #10126, and improve the logic in the next step.

If you're proposing #10137 as a workaround, I can understand. But the current implementation causes other troubles. So I'd like to disable it by default.

@asottile
Copy link
Contributor

asottile commented Jan 30, 2022

again, I'm not proposing to change how extlinks works -- just eliminating the warning in a common case

extlinks would still support slashes in replacements -- but from some searching on sourcegraph this feature seems to be scarcely or never used (I could not find an example in any projects from my search -- though I only scrolled through projects 5k stars and above)

the search I used:

:[^:\n]+:`[^`\n]+/[^`\n]+` file:\.rst$

so my proposal would leave the warning on (since I think the warning is useful -- it certainly helped me find a mistake in flake8) but it would eliminate a high false positive case that's almost always not intended to use extlinks

lethosor added a commit to DFHack/scripts that referenced this issue Jan 31, 2022
lethosor added a commit to DFHack/dfhack that referenced this issue Jan 31, 2022
justinmayer added a commit to getpelican/pelican that referenced this issue Feb 1, 2022
@Cielquan
Copy link
Contributor

I like the proposal from @gaborbernat and @asottile as this would silence most of the false positives for me too. But I still have an issue which would be better solved by a file exclusion list.

In the config.py I have extlinks={"repo": "https://github.com/user/some_repo/%s"}.
I include the CHANGELOG.md file from the repo's root in the docs. In the changelog I have a link to the releases page https://github.com/user/some_repo/releases and also compare links for each release like https://github.com/user/some_repo/compare/v1.0.0...main.

With the proposal the compare links would be silent, but the release link would still fire a warning. I do not want to replace any link in the changelog, because it should also be usable outside of the docs context.

@jodygarnett
Copy link

Just want to make a note that these warnings are unavoidable when mixing RST (documentation) and MarkDown (release notes) in a project.

  * Fix: [GEOS-10282](https://osgeo-org.atlassian.net/browse/GEOS-10282)

The markdown release notes generate this warning on extlink replacement:

WARNING: hardcoded link 'https://osgeo-org.atlassian.net/browse/GEOS-10282' could be replaced by an extlink (try using ':geos:`10282`' instead)

This is a shame as I enjoyed being able to cut and paste markdown out of issue tracker.

@kmuehlbauer
Copy link

We have the same issue while pulling in .rst documents from project-root via include-directive.

As the included .rst should just render fine outside sphinx compiled docs, the recommended change emitted by the WARNING is just not usable for us. As @Cielquan pointed out a file-exclusion list would help to solve this for us.

rly added a commit to NeurodataWithoutBorders/nwb-overview that referenced this issue Mar 23, 2022
The warning is overaggressive: 
sphinx-doc/sphinx#10112 and RST does not 
support nesting of directives/prefixes so we cannot use both 
:bdg-link-primary: and :nwb_extension_git:
@tk0miya tk0miya reopened this Mar 27, 2022
@tk0miya tk0miya modified the milestones: 4.5.0, 5.0.0 Mar 27, 2022
@ssbarnea
Copy link
Contributor

Maybe I am missing something but seeing that no1 regression I observed as being introduced by Sphinx 4.4.0, not only as not being fixed in 4.4.1 or 4.5.0, but postponed for 5.0.0.

That is something that will make people stick with older and older sphinx and avoid upgrading it.

@lethosor
Copy link

lethosor commented Mar 30, 2022

I'm no longer seeing the warning in Sphinx 4.5.0, and there is a mention of a change in the 4.5.0 changelog. So that part appears to have been resolved. I assume the issue remains open to track more complex future configuration support.

@tk0miya tk0miya modified the milestones: 5.0.0, 5.x May 7, 2022
@AA-Turner AA-Turner modified the milestones: 5.x, 6.x, 5.3.0 Oct 4, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.