Skip to content

Integrate sphinx's linkcheck#968

Merged
pradyunsg merged 28 commits intopypa:mainfrom
dukecat0:linkcheck
Aug 31, 2021
Merged

Integrate sphinx's linkcheck#968
pradyunsg merged 28 commits intopypa:mainfrom
dukecat0:linkcheck

Conversation

@dukecat0
Copy link
Copy Markdown
Member

@dukecat0 dukecat0 commented Aug 27, 2021

The output.txt will be generated in build/check/.

Comment thread locales/messages.pot
@@ -8,7 +8,7 @@ msgid ""
msgstr ""
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.

plz don't include updates to this file in the PR

Copy link
Copy Markdown
Member Author

@dukecat0 dukecat0 Aug 27, 2021

Choose a reason for hiding this comment

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

This is generated by Github Action. The workflow is running in fork.

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.

Interesting. Could you disable the workflow in your fork?

Copy link
Copy Markdown
Member Author

@dukecat0 dukecat0 Aug 27, 2021

Choose a reason for hiding this comment

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

Now I've disabled it. However, folks may still have this file in their PR if they didn't disable the workflow. Is there any method to disable it for all forks?

Copy link
Copy Markdown
Member Author

@dukecat0 dukecat0 Aug 27, 2021

Choose a reason for hiding this comment

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

Maybe we could add if github.repository=='pypa/packaging.python.org' to our workflow?

I found this here: https://github.community/t/stop-github-actions-running-on-a-fork/17965/2

For reference: https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#github-context

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.

What if the contributors would actually want to run workflows?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good question. I guess they don't really need to run this workflow in their forks? Since we could run this in our repo once PR is merged.

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.

Some people push branches to forks and check if they are fine before creating a PR, though.

Comment thread noxfile.py Outdated
Comment thread .gitignore Outdated
@dukecat0 dukecat0 requested a review from webknjaz August 27, 2021 14:43
Comment thread noxfile.py Outdated
Comment thread noxfile.py
dukecat0 and others added 3 commits August 28, 2021 16:13
Comment thread noxfile.py
dukecat0 and others added 2 commits August 29, 2021 00:43
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@dukecat0 dukecat0 requested a review from webknjaz August 29, 2021 09:09
Comment thread .github/workflows/test.yml Outdated
@dukecat0 dukecat0 requested a review from webknjaz August 29, 2021 10:31
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread source/conf.py Outdated
@webknjaz
Copy link
Copy Markdown
Member

Re: broken links — I think it's acceptable to ignore all of the failures and then fix them in a follow-up, unless you want to do that right here.

dukecat0 and others added 6 commits August 30, 2021 21:14
Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
@dukecat0 dukecat0 requested a review from webknjaz August 30, 2021 13:19
@dukecat0
Copy link
Copy Markdown
Member Author

@webknjaz Sorry for the late response. I've turned it into a matrix and seems it works.

Re: broken links — I think it's acceptable to ignore all of the failures and then fix them in a follow-up, unless you want to do that right here.

I prefer fixing them in a follow-up. :)

Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
@webknjaz
Copy link
Copy Markdown
Member

I prefer fixing them in a follow-up. :)

Then add them to ignores so that the job is green.

@webknjaz
Copy link
Copy Markdown
Member

@di could you make the linkcheck check required?

Comment thread .github/workflows/test.yml
@dukecat0
Copy link
Copy Markdown
Member Author

Then add them to ignores so that the job is green.

Probably I should fix them in this PR. The list is quite long.

@webknjaz
Copy link
Copy Markdown
Member

Probably I should fix them in this PR. The list is quite long.

There's 28 entries. I think it would be more effective should merge and fix them later.

@pradyunsg
Copy link
Copy Markdown
Member

I think it would be more effective should merge and fix them later.

I'm inclined to agree.

@pradyunsg pradyunsg merged commit 1eb79aa into pypa:main Aug 31, 2021
@dukecat0 dukecat0 deleted the linkcheck branch August 31, 2021 10:43
@pradyunsg
Copy link
Copy Markdown
Member

pradyunsg commented Aug 31, 2021

@meowmeowmeowcat I suggest you rebase PRs, instead of merging main repeatedly to have a cleaner history -- see https://akrabat.com/the-beginners-guide-to-rebasing-your-pr/.

This also enables you to have more incremental workflows as well, like using fixup commits with git rebase: https://jordanelver.co.uk/blog/2020/06/04/fixing-commits-with-git-commit-fixup-and-git-rebase-autosquash/ (which isn't necessary for most open source projects, but could be useful in professional environments).

@dukecat0
Copy link
Copy Markdown
Member Author

@pradyunsg Thanks for your suggestion!

@webknjaz
Copy link
Copy Markdown
Member

I think it would be more effective should merge and fix them later.

I'm inclined to agree.

Urgh... I was hoping to merge this with the links added to linkcheck_ignore so we could have that job marked as required right away.

@meowmeowmeowcat I suggest you rebase PRs, instead of merging main repeatedly to have a cleaner history

Honestly, I gave up long ago on having a clean history in this repository. Most of the contributors haven't had proper Git training anyway. But yeah, those "foxtrot merges" are sometimes annoying. OTOH I do prefer a history that actually represents the order of events and the flow of thoughts.

@meowmeowmeowcat if you want to learn to keep the history nice and clean, you probably need to get yourself familiar with interactive rebases. And maybe git-worktree if you tend to work on several PRs in the same project simultaneously.

If you want to go through more aspects of working with Git like a pro, here's a list of resources I have:

@dukecat0
Copy link
Copy Markdown
Member Author

Urgh... I was hoping to merge this with the links added to linkcheck_ignore so we could have that job marked as required right away.

It's ok. I will open a follow-up for broken links in few days and then we can mark the job as required in it. :)

@webknjaz Thanks for your resources! I found some of them are quite interesting. I'll take some time on reading them. 👍

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.

5 participants