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

Fix linting #124

Merged
merged 4 commits into from
Jan 30, 2025
Merged

Fix linting #124

merged 4 commits into from
Jan 30, 2025

Conversation

dcarley
Copy link
Contributor

@dcarley dcarley commented Jan 30, 2025

Fixes some issues that were described in this thread:

flox: Upgrade markdownlint and lychee

I want gitignore support from markdownlint-cli2 but it wasn't
available in the version that was pinned by python.version in the
toplevel package group.

I've moved lychee to a new group at the same time because it felt
weird having a group for a single package and I can safely verify that
these don't introduce any breaking changes.

ci: Move markdownlint globs to config

This fixes a bug whereby the globs were being incorrectly expanded by
the shell before being passed to markdownlint-cli2 because they
weren't quoted as the docs suggest.

Depending on the shell in use this could cause it to:

  • incorrectly miss nested directories like docs/tutorials/migrations
  • semi-correctly miss docs/reference/command-reference if the glob was
    expanded before flox activate copies the dir from flox/flox

lint: Ignore gitignored dirs (command-reference)

We don't want to lint docs/reference/command-reference because the
files are already committed and copied from flox/flox. We may choose
to apply linting there separately.

We could use !docs/reference/command-reference in the glob patterns
but it feels more reliable to depend on the gitignore rules that we
already have in case the path changes at any time.

lint: Auto-fix omitted migrations dir

These were inconsistently linted until a recent commit that moved the
globbing to the config. This is the result of running:

markdownlint-cli2 --fix

I want `gitignore` support from `markdownlint-cli2` but it wasn't
available in the version that was pinned by `python.version` in the
`toplevel` package group.

I've moved `lychee` to a new group at the same time because it felt
weird having a group for a single package and I can safely verify that
these don't introduce any breaking changes.
This fixes a bug whereby the globs were being incorrectly expanded by
the shell before being passed to `markdownlint-cli2` because they
weren't quoted as the docs suggest.

Depending on the shell in use this could cause it to:

- incorrectly miss nested directories like `docs/tutorials/migrations`
- semi-correctly miss `docs/reference/command-reference` if the glob was
  expanded before `flox activate` copies the dir from `flox/flox`
We don't want to lint `docs/reference/command-reference` because the
files are already committed and copied from `flox/flox`. We may choose
to apply linting there separately.

We could use `!docs/reference/command-reference` in the glob patterns
but it feels more reliable to depend on the gitignore rules that we
already have in case the path changes at any time.
These were inconsistently linted until a recent commit that moved the
globbing to the config. This is the result of running:

    markdownlint-cli2 --fix
@floxbot floxbot added the documentation Improvements or additions to documentation label Jan 30, 2025
Copy link

@dcarley dcarley requested review from stahnma and garbas January 30, 2025 14:21
Copy link
Contributor

@garbas garbas left a comment

Choose a reason for hiding this comment

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

❤️

@garbas garbas merged commit 3fca571 into preview Jan 30, 2025
4 checks passed
@garbas garbas deleted the dcarley/fix-linting branch January 30, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants