[#218] Change redirects default behaviour

[aeqz]

Description

Problem: Xrefcheck currently always follows redirect links.

Solution: We are changing its default behaviour regarding redirect links to fail and report permanent redirects, and to pass for temporary redirects. Further PRs will allow the user to configure other policies.

Related issue(s)

Fixes #218
Relates #25

✅ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this
checkmark indicating that you are sure it is dealt with (be that by irrelevance).

Related changes (conditional)

• Tests

  • If I added new functionality, I added tests covering it.
  • If I fixed a bug, I added a regression test to prevent the bug from
    silently reappearing again.

• Documentation

  • I checked whether I should update the docs and did so if necessary:
    • README
    • Haddock

• Public contracts

  • Any modifications of public contracts comply with the Evolution
    of Public Contracts policy.
  • I added an entry to the changelog if my changes are visible to the users
    and
  • provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

• My commits comply with the policy used in Serokell.
• My code complies with the style guide.

[Martoon-00]

Looks good, thank you 👍

Leaving a couple of insignificant concerns, and a medium one.

[Martoon-00]

Also, if you claim you resolve #218, it should be appropriate to add a test. We will touch the redirection logic a lot in the next PRs, and there is a chance to forget about #218 and leave no way to handle that issue.

Feel free to resolve this here or in a separate pull request (in the latter case, you can leave Relates #218 too).

[Martoon-00]

Oh, and one more thing: make sure to fit commit descriptions into 80 characters.

[Martoon-00]

Nice to see tests added 👍 I think you can add a tick to the PR description.

Leaving one more comment, would like to know your opinion.

[Martoon-00]

This is only my opinion, but I believe there is no reason to provide that high coverage, since e.g. handling of 302 and 303 in the code is exactly the same. E.g. having 302 w/ loc, 302 w/o loc, 307 w/ loc would be sufficient to cover the temporary redirects.

Using formal terms, we don't usually practice automated "black box" testing when the implementation is only one and concrete.

What do you think?

[YuriRomanowski]

I think that it's OK to check all the possible variants. But it seems that the way these tests are written is a bit verbose. We could for example make a separate function which tests two options (location present/absent) for a given code, and pass to this function as a parameter whether these tests should succeed or not. I believe that the code will get much shorter this way.

[Martoon-00]

If test cases can be turned into one- or two-liners, that approach sounds good.

[aeqz]

If we are not doing "black box" testing that is right, the three cases mentioned by @Martoon-00 would provide enough coverage. However, let's try to cover all the possible variants with a more concise style.

[Martoon-00]

Looks nice 👍

[Martoon-00]

@aeqz The comment of @YuriRomanowski seems to be also addressed, so in theory, you can merge this PR.

But indeed it is better to wait for his approval or a "feel free to merge without my approval" message.

[Martoon-00]

Don't forget to squash your commits after @YuriRomanowski's review is finished, as the 2nd and 3rd seem to be just additions to the 1st commit.