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

Add script to check translation slugs match English slugs #396

Merged
merged 4 commits into from
Apr 29, 2022
Merged

Add script to check translation slugs match English slugs #396

merged 4 commits into from
Apr 29, 2022

Conversation

delucis
Copy link
Member

@delucis delucis commented Apr 28, 2022

Follow up PR to #365. @FredKSchott suggested:

a lint script that warns if a language file (ex: /de/foobar.md) doesn't have a matching English file (ex: /en/foobar.md). We could run this in CI to make sure all PRs are passing.

This PR adds:

  • A script that checks slugs as proposed above
  • A GitHub action that will run this linter & the broken link checker for PRs and commits to the main branch

This slug linter will error in CI if it finds a slug that doesn’t have an English counterpart. The broken link checker currently just logs its output and won’t error (we can think about changing that once the 41 broken links it reports currently have been cleaned up).

@netlify
Copy link

netlify bot commented Apr 28, 2022

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit bb656c7
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/626bec79829364000809185b
😎 Deploy Preview https://deploy-preview-396--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@hippotastic
Copy link
Contributor

Looks good! I've tested it locally and it seems to work well.

The only nitpick I have is that you're chaining both linters with && in package.json, and that will cause the linkcheck linter to be skipped in case the slug linter finds any errors. I'd rather like to have separate tasks in the CI workflow that can fail independently from each other.

I'd suggest defining three jobs in ci.yml instead: One build job that performs the build process needed for any linters that may need it in the future, and two lint jobs. Here's an example implementation:

name: CI

on:
  push:
    branches: [main]
  pull_request:
    branches: [main]

# Automatically cancel in-progress actions on the same branch
concurrency:
  group: ${{ github.workflow }}-${{ github.event_name == 'pull_request_target' && github.head_ref || github.ref }}
  cancel-in-progress: true

defaults:
  run:
    shell: bash

jobs:
  # Build the docs using `pnpm run build`, and then upload `dist/` as a build artifact,
  # allowing any dependent jobs to use the output without having to re-run the build.
  build:
    name: Build Docs
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v3

      - name: Setup PNPM
        uses: pnpm/action-setup@v2.2.1

      - name: Setup Node
        uses: actions/setup-node@v3
        with:
          node-version: 16
          cache: 'pnpm'

      - name: Install dependencies
        run: pnpm install

      - name: Build Docs
        run: pnpm run build

      - name: Compress Build Output
        run: tar -czf dist.tar.gz dist

      - name: Upload Build Output
        uses: actions/upload-artifact@v3
        with:
          name: dist
          path: dist.tar.gz
          if-no-files-found: error

  # Run `pnpm run lint:linkcheck:nobuild` on the build output generated by the `build` job
  linkcheck:
    name: Check for Broken Links
    runs-on: ubuntu-latest
    needs:
      - build
    steps:
      - name: Checkout
        uses: actions/checkout@v3

      - name: Setup PNPM
        uses: pnpm/action-setup@v2.2.1

      - name: Setup Node
        uses: actions/setup-node@v3
        with:
          node-version: 16
          cache: 'pnpm'

      - name: Install dependencies
        run: pnpm install

      - name: Download Build Output
        uses: actions/download-artifact@v3
        with:
          name: dist

      - name: Extract Build Output
        run: tar -xzf dist.tar.gz dist
      
      - name: Lint
        run: pnpm run lint:linkcheck:nobuild

  # Run `pnpm run lint:slugcheck` (this job does not depend on the build and can run in parallel)
  slugcheck:
    name: Check for Mismatching Slugs
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v3

      - name: Setup PNPM
        uses: pnpm/action-setup@v2.2.1

      - name: Setup Node
        uses: actions/setup-node@v3
        with:
          node-version: 16
          cache: 'pnpm'

      - name: Install dependencies
        run: pnpm install

      - name: Lint
        run: pnpm run lint:slugcheck

@hippotastic
Copy link
Contributor

Additional note: In the future, our broken link checker should also set an exit code of 1 in case any broken links were found.

I'm currently not doing this because we have several existing broken links in the docs, and we probably don't want the PR checks to fail every time. This will be solved once I've updated the broken link checker to keep its state. Once we have access to the current state of broken links in the main branch, we can then compare the broken links in a PR to those, and only fail the PR check if NEW broken links were introduced.

@delucis
Copy link
Member Author

delucis commented Apr 29, 2022

Thanks @hippotastic! What do you think about focusing this PR only on running the slug checker and then you can add the link checker as part of your PR? Would that make most sense?

I also wonder if we can avoid some of the duplicate work between the tasks above (all the checkout, setup, dependencies etc.), I’ll look into that.


Aside: I’d actually be +1 on always failing on broken links once we’ve done a clean sweep once rather than adding the complexity of checking what’s new.

@hippotastic
Copy link
Contributor

Thanks @hippotastic! What do you think about focusing this PR only on running the slug checker and then you can add the link checker as part of your PR? Would that make most sense?

If I'm understanding you correctly, you're proposing to limit the new CI workflow to only running the slug check for now. That sounds good to me!

I also wonder if we can avoid some of the duplicate work between the tasks above (all the checkout, setup, dependencies etc.), I’ll look into that.

I've already researched this, no need to do it twice! 😄 GitHub allows both reusing workflows and actions, and you can see an example of how it works in my fork of the repo. I'll introduce this in my upcoming PR.

@delucis
Copy link
Member Author

delucis commented Apr 29, 2022

Awesome! OK, I’ll update this to just run the slug check for now and leave the rest to you. Thanks 😄

@FredKSchott
Copy link
Member

LGTM!

@FredKSchott FredKSchott merged commit 21be25d into withastro:main Apr 29, 2022
@delucis delucis deleted the delucis/slug-linter branch April 29, 2022 15:57
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.

3 participants