mckinsey / vizro

Vizro is a toolkit for creating modular data visualization applications.
https://vizro.readthedocs.io/en/stable/
Apache License 2.0
2.73k stars 142 forks source link

Apply code formatting to code examples in our docs #719

Open antonymilne opened 2 months ago

antonymilne commented 2 months ago

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

Dljdd commented 1 month ago

Hi I can help with this

stichbury commented 3 weeks ago

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 commented 2 weeks ago

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 https://github.com/adamchainz/blacken-docs/issues/357 for reference

antonymilne commented 2 weeks ago

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:

I'd suggest:

stichbury commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/KyleKing/mdformat-mkdocs/issues/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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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:

antonymilne commented 2 weeks ago

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

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.