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

[Docs] Add vale ini and styles for docs linting #385

Merged
merged 13 commits into from
Mar 27, 2024
Merged

Conversation

stichbury
Copy link
Contributor

@stichbury stichbury commented Mar 25, 2024

Description

This PR adds the .vale files to the /docs folders of vizro-core and vizro-ai so that I can manually run vale on docs pages as I work on them.

At some point I need to work with @l0uden to add this to our CI. We can do this as part of the PR but equally, as long as these files are in place, I can just invoke Vale for now.

Screenshot

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@stichbury stichbury added the Docs 🗒️ Issue for markdown and API documentation label Mar 25, 2024
@stichbury stichbury self-assigned this Mar 25, 2024
@stichbury
Copy link
Contributor Author

I'm not sure what the problem with the vizro-ai docs build is but I haven't changed anything in the docs build, so will investigate separately. It may be that we can/should put a single set of Vale files in the root of vizro to avoid duplication across core and ai folders?

@maxschulz-COL
Copy link
Contributor

Yes, ideally we would only have one folder with those files!

How are you using it, how is it invoved? Can we add it to pre-commit such that it gets invoked with our hatch run lint command. Then it should be not a problem in CI either, as this is done automatically!

@stichbury
Copy link
Contributor Author

How are you using it, how is it invoved? Can we add it to pre-commit such that it gets invoked with our hatch run lint command. Then it should be not a problem in CI either, as this is done automatically!

At present, I'm just invoking vale from the same folder as vale.ini and pointing at an individual file or folder to target. It's probably worth thinking how we want to use it across our docs -- we could either take the big hit of fixing everything in one, and then run across the full docs, or we could apply it just to any markdown file in a PR, which I think is what Kedro do https://github.com/kedro-org/kedro. I don't really mind which as long as it's available to check anything I'm working on. I think it can become annoying (it's still not fully tweaked for our purposes, so will be moaning about things) so I'm happy to spend more time on the styles to get them adjusted to Vizro lexicon.

As I'm not an engineer and not very well-versed in setting up this kind of tooling, please can I pass this one over to whoever could finish the PR from that perspective?

If you want to run across the full docs set, I'll need to put some time in place to read the issues and tweak styles or docs to get things passing.

@maxschulz-COL
Copy link
Contributor

How are you using it, how is it invoved? Can we add it to pre-commit such that it gets invoked with our hatch run lint command. Then it should be not a problem in CI either, as this is done automatically!

At present, I'm just invoking vale from the same folder as vale.ini and pointing at an individual file or folder to target. It's probably worth thinking how we want to use it across our docs -- we could either take the big hit of fixing everything in one, and then run across the full docs, or we could apply it just to any markdown file in a PR, which I think is what Kedro do https://github.com/kedro-org/kedro. I don't really mind which as long as it's available to check anything I'm working on. I think it can become annoying (it's still not fully tweaked for our purposes, so will be moaning about things) so I'm happy to spend more time on the styles to get them adjusted to Vizro lexicon.

As I'm not an engineer and not very well-versed in setting up this kind of tooling, please can I pass this one over to whoever could finish the PR from that perspective?

If you want to run across the full docs set, I'll need to put some time in place to read the issues and tweak styles or docs to get things passing.

Ok I see :)

Can you estimate how long the full pass would take? I think if we make this PR the introduction of vale, then there is no harm in it sitting here a bit longer. Ultimately I would like it to be another linter, because this is the only way to achieve consistency. Let me take a quick look what we can do, can I work into this PR?

@stichbury
Copy link
Contributor Author

@maxschulz-COL Yes please feel free to commit on this branch and take it on. I think a full pass would be less than a day of tweaking.

@maxschulz-COL
Copy link
Contributor

@maxschulz-COL Yes please feel free to commit on this branch and take it on. I think a full pass would be less than a day of tweaking.

Ok then let's go for that option, I will setup the pre-commit version of it

@maxschulz-COL
Copy link
Contributor

@stichbury I think you are good to go - vale can be invoked via hatch run lint, and it will also do so in CI :) Let me know if there are still any issues.

So the plan is to fix the existing vale errors, warnings and suggestions, tweak the setting a little, and then have our docs perfectly styled all the way 😎 ?

@maxschulz-COL
Copy link
Contributor

@stichbury Please double check if I messed up anything with the styles folder - I just copied over the one from the vizro-core, it's now all in a central place

@stichbury
Copy link
Contributor Author

Thank you, that is all fine! It all looks great, and I'll make a branch from this one to fix up the docs errors and tweak styles to get all building cleanly as we go forward. Thanks for making the changes to get it in the tooling!

@huong-li-nguyen huong-li-nguyen changed the title [docs] Add vale ini and styles for docs linting [Docs] Add vale ini and styles for docs linting Mar 27, 2024
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

@stichbury - could you click on the checkbox here :D

For context, we reduced this check to only one checkbox at some point but the compromise was that we always have to check that the person raising the PR is checking the checkbox, otherwise it shouldn't be merged 😄

Screenshot 2024-03-27 at 10 18 36

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

LGTM! But please do consider the three remaining open points before merging:

  • changelogs
  • necessity of added files compared to e.g. Kedro
  • clicking the consensus button

@huong-li-nguyen huong-li-nguyen self-requested a review March 27, 2024 11:09
@stichbury stichbury enabled auto-merge (squash) March 27, 2024 11:12
@stichbury stichbury merged commit 74ee3b5 into main Mar 27, 2024
33 checks passed
@stichbury stichbury deleted the docs/add-vale-styles branch March 27, 2024 11:14
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants