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-infra] Share vale-config #41176

Merged
merged 10 commits into from Feb 21, 2024
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Feb 19, 2024

Implemented

The common style rules are in docs/writing-rules To update them, you need to modify the files, and run pnpm run docs:zipRules that will update the writing-rules.zip which is used by vale (for this repo and others)

Also added in this PR:

  • Some rules for charts
  • the correction 'server side' -> 'server-side' and 'client side' -> 'client-side' from toolpad

Initial message

I would like to create a vale package to get a single source of truth about our writing style. For now, we need to duplicate PR to sync core and X

I planed to use the monorepo trick, but the vale action does not install dependencies so the monorepo is not added.

We could:

Option 1

Add a yarn install phase in vale action do download the monorepo

I'm not in favor of this one since it's downloading lost of thing just to get few files

Option 2

Move vale rules to a distinct repository such that it could have a release that generate a .zip files

The drawback of this option is that you can't directly test modification on the docs. But we could create few markdown documents for testing purpose

Option 3

Have a storage option to publish and download a zip file containing the syntax rules.

@mui/code-infra I don't know if it resonates with some of your discussion. I assume no, because this is one of the rare piece of the repo that is not JS

@mui-bot
Copy link

mui-bot commented Feb 19, 2024

Netlify deploy preview

https://deploy-preview-41176--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 702d39b

@alexfauquette alexfauquette added the scope: docs-infra Specific to the docs-infra product label Feb 19, 2024
@Janpot
Copy link
Member

Janpot commented Feb 19, 2024

option 4

Store the zip file in the repository and update it manually before pushing changes with a script. Have a CI check that verifies the zip is up to date. Then use raw github link to load in dependent repos. (Q: how big is this zip file expected to be?)

@alexfauquette
Copy link
Member Author

alexfauquette commented Feb 19, 2024

@Janpot It's currently a baby of 3.1kB obtained by runnign zip -r "writing-rules.zip" docs/writing-rules on this branch

@Janpot
Copy link
Member

Janpot commented Feb 20, 2024

@Janpot It's currently a baby of 3.1kB obtained by runnign zip -r "writing-rules.zip" docs/writing-rules on this branch

thanks, seems reasonable enough to store it in the repo itself imo

@alexfauquette alexfauquette marked this pull request as ready for review February 20, 2024 16:23
@alexfauquette
Copy link
Member Author

@Janpot I did not add the check to see if the zip is up to date, because I'm wondering if the zip will be independent on the system generating it, and I made vale use directly the zip such that the zip file is the source of truth (not the yml files)

@@ -2,20 +2,14 @@
StylesPath = .github/styles
MinAlertLevel = suggestion

Packages = Google
# The docs/writing-rules.zip is generated by `pnpm docs:zipRules`
Packages = Google, docs/writing-rules.zip
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to embed the Google package in docs/writing-rules.zip?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did not tried. Maybe

What would be the usage of adding Google here? Simplifying the .vale.ini to just importing this zip?

Copy link
Member

@Janpot Janpot Feb 20, 2024

Choose a reason for hiding this comment

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

Simplifying the .vale.ini to just importing this zip?

Yes, making the setup on dependent repos as small as possible. I mean, I'm assuming that the Google guide is what we start from for our own writing style.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we started with the basics Google rules we wanted to pick.

I prefer to keep them distinct, such that in material-ui repo we store the rules, and then each sub repo are able to configure its vale as they want by allowing or not different rules

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to keep them distinct, such that in material-ui repo we store the rules, and then each sub repo are able to configure its vale as they want by allowing or not different rules

Is it possible for material-ui repo to provide the Google defaults and X/Toolpad override where necessary? That would allow @samuelsycamore to maintain the these defaults in a single location as well.

Copy link
Member

Choose a reason for hiding this comment

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

In the interest of consistency, I think it would be preferable not to have overrides at the product level—any style rules we use should be applied globally. I can't think of a reason why one product would add or remove rules that shouldn't also apply to all of the other products.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I created #41225 to don't forget. I will take care of it after cleaning all the PR already open :)

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

@Janpot I did not add the check to see if the zip is up to date, because I'm wondering if the zip will be independent on the system generating it, and I made vale use directly the zip such that the zip file is the source of truth (not the yml files)

👍 as long as there are good instructions. Another option is to unpack the zip and compare the result with the original folder.

@alexfauquette alexfauquette merged commit fdb8cdf into mui:master Feb 21, 2024
22 checks passed
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 17, 2024

I think we should go with a variant of Option 1: install the one dependency we need.

I don't think it will work in the current setup as I'm moving to implement "Causes CI/CD jobs to fail" https://docs.gitlab.com/ee/development/documentation/testing/vale.html#result-types. Either we use HEAD but we lose idempotence ❌, or we set a tag but we have to remember to update it ❌.

@Janpot
Copy link
Member

Janpot commented Mar 17, 2024

or we set a tag but we have to remember to update it ❌.

I believe this can be automated with renovatebot https://docs.renovatebot.com/presets-regexManagers/

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 17, 2024

@Janpot it's another degree of freedom, eww, but if this is batched in renovate, and it means the Vale step runs in 40s rather than 120s, it could be worth it.

If we ever need to run pnpm in the GitHub Actions, maybe like in Circle CI it makes sense to have a shared cache. We do for instance here https://github.com/mui/mui-x/blob/0dc41910f86e11cef96d0f1db7b1af8cfbcc8a63/.github/workflows/l10n.yml#L26. It costs about 60s.

In any case, I think think we can wait for the problem to happen. Right now, I'm only working on failing the CI for the diffs that are in the PR, not for the whole codebase #40944. So it feels like something we should be lazy about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants