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

lint: consider scope of markdown linting in configlet #562

Open
ee7 opened this issue Apr 15, 2022 · 3 comments
Open

lint: consider scope of markdown linting in configlet #562

ee7 opened this issue Apr 15, 2022 · 3 comments
Labels
cmd: lint kind: design Discussing overall direction

Comments

@ee7
Copy link
Member

ee7 commented Apr 15, 2022

Follow-up on some discussions with Erik, starting from implementation options for #328.

I suggested that for a first implementation of markdown linting, we could consider adding markdownlint as an org-wide workflow (rather than adding it to configlet lint).

The downside: running configlet lint locally runs a smaller subset of checks that occur in CI. But configlet lint is not supposed to be the only CI check for every track, and even for configlet lint JSON checking we might add CI-only checks anyway (e.g. for Exercise-wide checks - see #178).


@ee7 wrote:

There is some question about how to separate things. For a simpler example, it's probably nice to have org-wide whitespace checking. So should configlet lint do that, or an org-wide workflow?

There we might argue that:

  1. The scope of configlet lint is track repos
  2. Exercism has more than just track repos
  3. Other repos might have whitespace problems
  4. Therefore whitespace checking should be done outside of configlet lint

Regarding the markdownlint option, I guess it depends on how many rules we have that markdownlint cannot catch. That is, things that are valid markdown, but violate some Exercism-specific linting rule.

By the same reasoning, it makes sense to have the JSON linting in configlet lint (as we have already), because:

  1. The scope of configlet lint is track repos
  2. Only track repos have certain JSON files
  3. Parsing JSON (and confirming that it's valid) is easy
  4. We're checking lots of Exercism-specific rules that are not about JSON validity, and are even difficult/impossible to express in e.g. JSON schema
  5. With those files, it's more useful to have fast local linting without installing dependencies (which configlet provides)

But if we want lots of the checks that markdownlint provides, it's probably a significant amount of work to essentially reimplement markdownlint in pure Nim

@ee7 ee7 added kind: design Discussing overall direction cmd: lint labels Apr 15, 2022
@ee7
Copy link
Member Author

ee7 commented Apr 15, 2022

Otherwise, the main available Markdown libraries seem to be:

I haven't used either. It is also possible to wrap some other C library.

@ErikSchierboom
Copy link
Member

nim-cmark requires that libcmark is installed and the shared library is available.

That does mean that we would be adding an extra prerequisite, right?

Having slept on it a bit, I think having a GitHub action for Markdown linting might be a more sane option. There are several benefits to it:

The downsides are:

  • Maintainers might have to do work even though configlet tells them everything is fine
  • We don't integrate with configlet fmt

A hybrid approach would be to have an API method that configlet could call to format Markdown, but that seems quite wasteful.

@ee7
Copy link
Member Author

ee7 commented Apr 22, 2022

That does mean that we would be adding an extra prerequisite, right?

We might be able to link it statically.

I think having a GitHub action for Markdown linting might be a more sane option.

To others: note that the configlet repo has markdownlint in CI:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd: lint kind: design Discussing overall direction
Projects
Status: No status
Development

No branches or pull requests

2 participants