Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Link checker: deduplicate links #2923

Merged
merged 3 commits into from
Sep 2, 2019

Conversation

kodebach
Copy link
Member

Changes the linkchecker so it de-duplicates before checking them (reduces from ~1100 to ~800 checks). In addition the timeout was reduced from 10s to 5s and the number of retries from 20 (default) to 3.

Basics

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see Coding Guidelines)
  • meta data is updated (e.g. README.md of plugins and METADATA.ini)

Review

@markus2330
Copy link
Contributor

Thank you for the PR!

Is it maybe also useful to have a whitelist? (as workaround when some links are down?)

@kodebach
Copy link
Member Author

Is it maybe also useful to have a whitelist? (as workaround when some links are down?)

Probably, though I'm not sure if storing it in the repository is a good idea. I'll have to think whether there are any problems with that.

@markus2330
Copy link
Contributor

I was thinking about something like tests/icheck.suppression (Of course it needs to be empty before releases and not like icheck after.)

If we do not want it to be in the repo (which would require everyone to rebase so that the link checker succeeds) we can also put the file on doc.libelektra.org or similar.

@kodebach
Copy link
Member Author

kodebach commented Aug 31, 2019

I think it should work to have the whitelist in the repo without rebasing, since the link checker build job now merges PRs before checking.

However, the whitelist is another file that is likely to cause merge conflicts in PRs (like the release notes°, and icheck.suppression).

° For the release notes the main problem introducing the merge conflicts is that we start with a file full of placeholders that people overwrite.

@markus2330
Copy link
Contributor

You are right, it would be similar for the whitelist. But did we ever had a merge conflict in the icheck.suppression?

I am open to improvements for release notes. Afaik two different appends on a file will also be a merge conflict. What is not a merge conflict, is if different lines are edited, so having more TODO lines in the template might help if people use a random TODO (and not always the first).

@kodebach
Copy link
Member Author

But did we ever had a merge conflict in the icheck.suppression?

I had one one #2916.

if people use a random TODO (and not always the first)

Not sure that is going to happen. But we could use a custom merge driver for the release notes file.

@kodebach
Copy link
Member Author

kodebach commented Sep 1, 2019

I created #2925 for the discussion about the release notes. For the link checker, I will add a simple whitelist file. It shouldn't cause merge conflicts very often and if it turns out it does, we can use the union merge driver, as duplicated lines shouldn't be a problem in a whitelist.

@markus2330
Copy link
Contributor

Thank you! Yes, actually we can use the union merge driver also for icheck and release notes.

@markus2330
Copy link
Contributor

Thank you great job! Good to have this fixed!

Why is there the message "/tmp/cirrus-ci-build/doc/tutorials/highlevel-bindings.md:59:0 warning: invalid link 'code-generator.md'"?

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Good job!

@markus2330 markus2330 merged commit be9b67f into ElektraInitiative:master Sep 2, 2019
@kodebach
Copy link
Member Author

kodebach commented Sep 2, 2019

Why is there the message "/tmp/cirrus-ci-build/doc/tutorials/highlevel-bindings.md:59:0 warning: invalid link 'code-generator.md'"?

#2919 was merged before #2805, so the file doesn't exist yet.

@markus2330
Copy link
Contributor

Ahh ok, can you please revert cda9269 in #2927?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants