Fix Lychee link failures from stack overflow

[kratman]

Description

Removes stack overflow links due to lychee failures

Type of change

Fixes Lychee link checker

• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.59%. Comparing base (1a5c4e4) to head (99376b5).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3989   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        21310    21310           
========================================
  Hits         21223    21223           
  Misses          87       87           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[agriyakhetarpal]

I would be slightly against this change. Usually link checkers are used to prevent link rot and make sure that dead links are not scattered across codebases. However, StackOverflow permalinks will never go extinct as long as StackOverflow never goes down, so, could we ignore the website altogether?

[Saransh-cpp]

However, StackOverflow permalinks will never go extinct as long as StackOverflow never goes down, so, could we ignore the website altogether?

+1 for this.

Updating numpy and python links is a good idea.

[kratman]

@agriyakhetarpal, @Saransh-cpp In general I don't think adding links to code snippets is all that useful. I don't think many people would just copy the link to look at it. They would then have to read a whole blog post or forum post just to find the key pieces of information. If there is something that important in the link, then it should be explained in plain text comments. I strongly believe comments should be minimized, but their main purpose is to explain extremely complex parts of the code. If somebody changes a code block commented by a link, then the person updating that code block has to read the link to determine if they changed what was commented in there. This can easily lead to code rot in the comments.

While we can mostly assume that some sites will never change their domains or delete pages, the whole point of link checking is to confirm that the assumption is correct. If we suppress any site which is not perfect for the link checker, then what is the point of the link checker? There are some things like the paths to executables which the link checker will never be able to handle, but the point is to check if links work. If somebody adds a new stack overflow link in a comment do you think all reviewers are going to copy the link to check it or do you think they will see the link checker pass and assume that the link is correct?

Additionally the link checker has repeatedly marked PRs as failing their CI checks, which hides real problems. I am going to go back through and update the comments in the parts @tinosulzer flagged, but I think in general reducing the ways in which things can fail or break is an improvement.

[kratman]

Real link failure that was hidden by the StackOverflow ones:
https://github.com/keras-team/keras/blob/master/keras/callbacks/callback.py | Failed: Network error: Not Found

[agriyakhetarpal]

@kratman and I discussed this outside of GitHub recently and I had pitched for the creation of an automated issue in a workflow that runs fortnightly (or monthly would be better) instead of on PRs (which I would be happy to look at the failures for and be assigned to the issue, and also modify the workflows to do this). This is how 2i2c is doing it, and they do so monthly: 2i2c-org/2i2c-org.github.io#229. While it doesn't hide the problem of whether StackOverflow links should be included and they can get very outdated, this could serve as a reasonable alternative to let CI checks display as passing while having flaky links and therefore mitigate ❌s showing up before a ✅ does

I think the point of a link checker is more about verifying internal links (such as those in the docs, which we have #3387 open for) or for those links that are absolutely needed, so reducing the current ~540 links should also be great to do sometime or the other. This comment still doesn't resolve how to navigate the "providing-attribution-vs-removing-links-to-it" but I wish to advocate for reducing non-useful links but also keeping useful ones.

[kratman]

I had pitched for the creation of an automated issue in a workflow that runs fortnightly (or monthly would be better) instead of on PRs

I think it is better to have the link checker run on individual PRs if we are going to do it. The purpose should be to check if a new change has added a bad link

[kratman]

@valentinsulzer, @Saransh-cpp Can you re-review I would like to get this link checker fixed

[Saransh-cpp]

If somebody adds a new stack overflow link in a comment do you think all reviewers are going to copy the link to check it or do you think they will see the link checker pass and assume that the link is correct?

This is an issue, but I believe the person reviewing the PR will most probably look at the link to understand the implementation.

I would still prefer to keep the StackOverflow links. As I mentioned below, they're not useful for direct users but are somewhat useful for people trying to extend the software or depend on it as a library. The majority (maybe all) of the libraries that I work on attribute StackOverflow answers and even GitHub issues/discussions in comments. I know that I will definitely go through the links attached in the comments when I'm trying to understand a functionality on a deeper level.

I won't block the PR. Please feel free to merge if everyone else is happy with it.

[Saransh-cpp]

A code block is not necessarily always an MWE.

[Saransh-cpp]

This is the only comment that is a blocker from my end.

[kratman]

I opted to do it this way because explaining what a MWE is was quite wordy, so instead of even mentioning that, I just switched it to asking for them to put in code that produces the error.

[agriyakhetarpal]

Perhaps could we add the link to Wikipedia – https://en.wikipedia.org/wiki/Minimal_reproducible_example, it shouldn't fail as such? I checked it with lychee locally and it works as usual

[kratman]

But why even bother with a link when we can just use words

[agriyakhetarpal]

Makes sense, could you an extended description? Like not to be excessively wordy, but just another sentence about what additional context people can provide besides their snippets in code blocks

[Saransh-cpp]

I'll still prefer attributing StackOverflow answers. Moreover, in this case, the question was asked by @agriyakhetarpal on StackOverflow to specifically cater to the needs of PyBaMM.

[Saransh-cpp]

Can we replace this with the updated link? It is often useful to look at the original implementations, especially for people trying to extend PyBaMM (or any software) and not just use it.

[agriyakhetarpal]

Replacing with a GitHub permalink (pinned to a commit hash) would also be helpful, so that we can know what the implementation was at the time of writing and the link won't break.

[kratman]

I don't think we need to mark were we were inspired by other projects. Especially when concepts like callbacks are pretty common.

We should use links only where it is necessary. The fact is that this keras link broke just 6 months ago too and this time it was not appearing in the link checker log until the stackoverflow stuff was removed.

[Saransh-cpp]

Ditto!

[Saransh-cpp]

Ditto!

[kratman]

I added plain text that explains, so we don't need the link anymore. If the explanation is not good enough then we can just update the text

[valentinsulzer]

In the interest of closing this bikeshed PR, I'm happy to merge once the suggested change is made.

[valentinsulzer]

Thanks! @Saransh-cpp I think that addresses your main concern as well - just waiting for your approval now

[agriyakhetarpal]

I'm approving as well; it has been a recent revelation that content from StackOverflow links is brought under the CC-BY-SA license, which is not compatible with our BSD-3-Clause license without the due permission of the author of the code snippet being referenced. SciPy's development guide has more information. I would consider adding this to our Contributing Guide somewhere and it should be fine within this PR itself, @kratman – thanks!

Changes were made

[kratman]

I dismissed @Saransh-cpp's review since he said previously:

Please feel free to merge if everyone else is happy with it.