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

Apply code formatting to code examples in our docs #719

Open
antonymilne opened this issue Sep 18, 2024 · 14 comments
Open

Apply code formatting to code examples in our docs #719

antonymilne opened this issue Sep 18, 2024 · 14 comments
Labels
Docs 🗒️ Issue for markdown and API documentation Good first issue 🐤 Issue suitable for first-time contributors hacktoberfest A month-long celebration of all things open-source

Comments

@antonymilne
Copy link
Contributor

Currently our code examples are not formatted using black or linted in any way.

  • Investigate what mkdocs extensions there are to do this and what they would do (e.g. they might run ruff or black)
  • Find a good solution and apply it!
@antonymilne antonymilne added Docs 🗒️ Issue for markdown and API documentation Good first issue 🐤 Issue suitable for first-time contributors GHC: docs track labels Sep 18, 2024
@antonymilne antonymilne added hacktoberfest A month-long celebration of all things open-source and removed GHC: docs/code track labels Oct 9, 2024
@Dljdd
Copy link
Contributor

Dljdd commented Oct 14, 2024

Hi I can help with this

@stichbury
Copy link
Contributor

We could use blacken-docs but there's a problem (when isn't there?) which is that we don't surround our code with a standard lexer block anymore because we're using the MkDocs plugin for PyCafe. It may be possible to fork it and build our own version to check blocks for {.python pycafe-link} ??

Something I'm going to need some advice from @antonymilne or @maxschulz-COL on I think.

@maxschulz-COL
Copy link
Contributor

I have already reported on that and there is a ticket and even a PR that aims to finish it. I didn't get around to doing it though.

See adamchainz/blacken-docs#357 for reference

@antonymilne
Copy link
Contributor Author

I also looked into this and decided that mdformat was probably the best solution here. It will (in theory) lint not just our Python code in snippets but our markdown files in general and it seems have a well maintained plugin mdformat-mkdocs.

After playing around a bit I found that the right pre-commit configuration looks something like this:

- repo: https://github.com/executablebooks/mdformat
  rev: 0.7.18
  hooks:
    - id: mdformat
      args: [--ignore-missing-references, --wrap=no, --align-semantic-breaks-in-lists]
      additional_dependencies:
        - "mdformat-mkdocs[recommended]==3.0.0"

The problems were:

  • mdformat-ruff (need to check but maybe this isn't actually used by mkdformat-mkdocs) just does ruff format and not ruff check so doesn't do linting fixes. Maybe add this myself or wait for one integrated command: Unified command for linting and formatting astral-sh/ruff#8232
  • doesn't work with {.python pycafe-link}

I'd suggest:

  • @stichbury, given you're the owner of most of our markdown files, see what you think of the changes made by the above pre-commit config involving mdformat outside of code snippets
  • separately (and non-urgently) I and/or @maxschulz-COL will take a look into the code snippet formatting

@stichbury
Copy link
Contributor

I'd suggest:

  • @stichbury, given you're the owner of most of our markdown files, see what you think of the changes made by the above pre-commit config involving mdformat outside of code snippets

This is great, yes, I can do that. Will you make a branch for that pre-commit, or shall I? Happy to put this into the upcoming sprint to get it further advanced.

@antonymilne
Copy link
Contributor Author

You go for it 🙂 I suggest you start with this:

- repo: https://github.com/executablebooks/mdformat
  rev: 0.7.18
  hooks:
    - id: mdformat
      args: [--ignore-missing-references, --wrap=no, --align-semantic-breaks-in-lists]

and play around with the args to see if you like them or if there's any others that you like.

And then afterwards add in this to see what difference it makes:

      additional_dependencies:
        - "mdformat-mkdocs[recommended]==3.0.0"

@stichbury
Copy link
Contributor

I've given it a try here: https://github.com/mckinsey/vizro/tree/docs/update-precommit but two of the args you included (--ignore-missing-references and --align-semantic-breaks-in-lists) are returning errors for me. They don't seem to be documented in the mdformat docs? I'm happy with the word wrap changes so this would work fine for me if the code formatting is OK for you.

@antonymilne
Copy link
Contributor Author

Ah yes, I think those arguments are only added when "mdformat-mkdocs[recommended]==3.0.0" is added.

The changes are pretty far-reaching (one reason I didn't do this before as a quick PR) and I think will need some careful checking through, mainly to make sure that the built docs still look ok. e.g. I'm pretty sure that changes like
\[Container\]\[vizro.models.Container\] (see KyleKing/mdformat-mkdocs#19) and this will have broken things.

What happens if you just run it with this without the mdformat-mkdocs at all?

- repo: https://github.com/executablebooks/mdformat
  rev: 0.7.18
  hooks:
    - id: mdformat
      args: [--wrap=no]

(and playing around with any other settings you might like there)

@stichbury
Copy link
Contributor

Thanks!

My branch has the change for just the mdformat initiated wrap changes. There isn't anything else to set for mdformat since the only other parameter is for the end of file, which is covered by another lint tool.

I'll have another play with the mkdocs plugin since I couldn't get it working yesterday but maybe there's an extra space or something in my file.

@antonymilne
Copy link
Contributor Author

Isn't that the change caused by the following configuration though, which includes mdformat-mkdocs? This is what the pre-commit.yaml file looks like in the diff you link to:

  - repo: https://github.com/executablebooks/mdformat
    rev: 0.7.18
    hooks:
      - id: mdformat
        args: [--wrap=no]
    additional_dependencies:
      - "mdformat-mkdocs[recommended]==3.0.0"   

@stichbury
Copy link
Contributor

stichbury commented Nov 12, 2024

It doesn't seem to make a difference @antonymilne -- the changes are coming from mdformat and if you look at the order of commits for the PR:

  1. I added mdformat to the pre-commit config
  2. Linted to generate 81 changed files and committed them to the branch
  3. Added the additional dependency to the pre-commit config
  4. Linted again: no additional changes, committed the pre-commit config to branch

Ignore me -- I was correct earlier when I said "maybe there's an extra space or something in my file."

There was a missing 4 spaces 🤦‍♀️🤦‍♀️🤦‍♀️

@stichbury stichbury reopened this Nov 12, 2024
@stichbury
Copy link
Contributor

stichbury commented Nov 12, 2024

Whoops, didn't mean to close. OK, here's a new branch with the output from mdformat-mkdocs. I've noticed a few things already:

  • The changes to the API docs inclusion files break the docs and serve fails. I rolled them out
  • The changes to links to API docs are broken
  • There are a few files that mdformat is erroring out rather than changing

@antonymilne
Copy link
Contributor Author

Thanks @stichbury! I've spent a while looking through your branch and playing round myself.

  • API docs are best to just ignore so I exclude them
  • fixed all the errors (which were actual real errors with our yaml) apart from custom-components.md which I didn't look at yet

Please could you continue working on #873? And give me a shout if you need any help with it. I think this is probably worth proceeding with but I'm not wedded to it if it's seems like too much effort for little reward.

btw remember you can run just one linter with hatch run lint mdformat which is much faster than hatch run lint doing all of them.

@antonymilne
Copy link
Contributor Author

Next step here:
apply mdformat-ruff or mdformat-black or blacken-docs (preference is in that order), ideally with good line length. Needs to work for {.python} code blocks: easiest way is to do python {pycafe-link title="a"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs 🗒️ Issue for markdown and API documentation Good first issue 🐤 Issue suitable for first-time contributors hacktoberfest A month-long celebration of all things open-source
Projects
None yet
Development

No branches or pull requests

4 participants