chadfawcett Github contribution chart
chadfawcett Github Stats
chadfawcett Most Used Languages

Activity

27 Oct 2022

Issue Comment

Chadfawcett

Warning causes workflow to fail when `fail_on_error` is `true`

Based on the docs, I would only expect the workflow to fail if there was an error. I'm seeing it fail when there is only a Warning. Is this the expected behaviour?

According to the reviewdog docs, a Warning should result in a "neutral" GitHub Status.

Forked On 26 Oct 2022 at 04:23:12

Chadfawcett

Thanks for the response @jdkato. Here's my config setup

.vale.ini

MinAlertLevel = suggestion
StylesPath = .github/styles

[*.md]
BasedOnStyles = proselint, inclusive-language
proselint.Typography = NO
proselint.Cliches = NO
proselint.Very = NO
proselint.Cursing = NO
proselint.Currency = NO
proselint.CorporateSpeak = NO
proselint.Skunked = NO
proselint.DateSpacing = NO
proselint.DateCase = NO
proselint.Diacritical = NO
proselint.Hyperbole = NO 

workflow

name: Vale Lint
on:
  pull_request:

jobs:
  prose:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v2

      - name: Vale
        uses: errata-ai/vale-action@e9cd17e2bb87ed772b939286b7b141d7ba64ed54
        env:
          GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}
        with:
          fail_on_error: true 

Then the warning was for starting a paragraph with "but". Which is defined like so: .github/styles/proselint/But.yml

extends: existence
message: "Do not start a paragraph with a 'but'."
level: warning
scope: paragraph
action:
  name: remove
tokens:
  - ^But 

Commented On 26 Oct 2022 at 04:23:12
Issue Comment

Chadfawcett

Handle titles starting with numbers

Initial checklist

Problem

Note: Not sure if this is better solved here or in github-slugger. I decided to open here since I think a solution to this problem would make that package stray from it's intention of being as close to GitHub's slugger as possible. Happy to move over to that repo if you think it's a better fit there though.

I have a scenario where an author of some content has started the header with a number. This results in an id that starts with a number, which isn't valid according to the CSS spec (HTML spec was a little less clear whether it was valid or not). Regardless, when I tried to reference the header using document.querySelector, it throws an error saying I'm using an invalid selector.

Solution

A solution I could take on my end would be to append something to the beginning of these IDs and update the links, but I thought it'd be better to be done upstream (either in rehype-slug or github-slugger).

I created a quick solution for github-slugger where it just appends an underscore (_) to any id that'd start with a number. I figured I should reach out first before submitting a PR though. GitHub appends user-content- to all of the ids of in the rendered markdown. You can see this when inspecting the README of this project.

This is where I'm unsure of where this solution should be implemented. GitHub doesn't include the user-content- in the url produced for each header, but instead handles that client side. So github-slugger only sluggifies what comes after the user-content-. So if we start appending something like an underscore, it means that package may deviate too much from its goal.

For those reasons, I think it may be the responsibility of this package.

Alternatives

I touch on the alternatives in the Solutions section above, but I'll reiterate and summarize here:

  1. Handle it in my specific application
  2. Handle it in rehype-slug as I think this could be a problem for many people (not just me)
  3. Handle it in github-slugger if it doesn't force that project to deviate from it's goal of "emulate the way GitHub handles generating markdown heading anchors as close as possible."

Forked On 25 Oct 2022 at 05:52:10

Chadfawcett

Thanks for the detailed response @wooorm!

rehype-sanitize definitely seems like the way to go for most projects. Since this is an internal project, I'm going to just go with the getElementById/[id="3-xxx"] syntax so I don't have to worry about the caveats with the changing classes for the other plugins (linking, syntax highlighting, etc).

I’m open to adding an option here to prefix things.

I think the two solutions you provided resolve the need for this option. Hopefully this issue can help someone in the future!

Thanks again :)

Commented On 25 Oct 2022 at 05:52:10