-
-
Notifications
You must be signed in to change notification settings - Fork 570
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
#5341 code analysis documentation #5882
Merged
Merged
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
c4d8ce3
Updated linting docs with info on how to test linting on Volto core
ichim-david dbf5534
Renamed fixtures to specs or tests and provided more info on steps ne…
ichim-david 4a07688
Updated testing docs with info on how to run locally the test commands
ichim-david 33f666a
Added notice of checking tests when pushing commits and linking to ma…
ichim-david b81f2c4
Tidy up acceptance-tests.md
stevepiercy 02e22ad
Grammar fix
stevepiercy dbcf44e
Spacing
stevepiercy 2348fc2
Consolidate pnpm workspaces references
stevepiercy 3968aa3
More consolidation of pnpm workspaces
stevepiercy 6dbeaa2
More workspace clean-up
stevepiercy 3b7643c
Add term for `workspace`. See https://github.com/plone/documentation/…
stevepiercy 87433c3
Remove note about CI. It should have its own section.
stevepiercy be301f1
Clarify context of pnpm commands
stevepiercy f77f87e
Merge branch 'main' into 5341-code-analysis
stevepiercy a32b1ab
Revise linting.md
stevepiercy 00de448
Tidy up testing.md
stevepiercy 53a60d7
Add cross-reference
stevepiercy 475f0d9
Add more cross-references
stevepiercy 487dcce
Merge branch 'main' into 5341-code-analysis
ichim-david File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,17 +9,12 @@ myst: | |
|
||
# Linting | ||
|
||
```{note} | ||
This documentation is a work in progress. Any help is welcome to fill in the | ||
gaps! | ||
``` | ||
|
||
Volto developers can enjoy a lot of freedom in their choice of text editors and | ||
IDEs, thanks to the strong tooling provided by the JavaScript ecosystem. | ||
|
||
At the core of these capabilities is ESLint, the advanced JavaScript linting | ||
and formatting tool. Also included with Volto is integration with Stylelint and | ||
Prettier. | ||
At the core of these capabilities is `ESLint`, the advanced JavaScript linting | ||
and formatting tool. Also included with Volto is integration with `Stylelint` and | ||
`Prettier`. | ||
|
||
For Visual Studio Code you'll need to integrate an | ||
[ESLint plugin](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint). | ||
|
@@ -35,3 +30,60 @@ most importantly for .js/jsx files: | |
- The editor should automatically flag unused imports | ||
- The editor should automatically (and properly) flag imported modules that are not found | ||
- The editor should provide automatic code formatting on save | ||
|
||
## Linting Volto core | ||
If you want to contribute to Volto core you need to perform several quality control tasks in order to ensure that you commits are not breaking the automated tests that Volto is performing on every commit. | ||
|
||
The first step in the testing hierarchy is the performing of code linting. | ||
|
||
Volto core should already perform these linting commands when commiting locally from the [husky](https://typicode.github.io/husky/get-started.html) integration, however if the the automated check doesn't happen when performing a commit or you want to get less information you can also run each linting task manually as you will learn how-to from the next section. | ||
|
||
If you want to see exactly which linting commands are set to run after a commit have a look at the [.lintstagedrc](https://github.com/plone/volto/blob/main/.lintstagedrc) file from the Volto project [main](https://github.com/plone/volto/) GitHub branch. | ||
|
||
Volto core performs linting using the following commands: | ||
- eslint - For finding problems in the project's code files | ||
- prettier - For uniform code formatting | ||
- stylelint - For uniform style formatting | ||
|
||
Although we can run the linting commands from the root of the project | ||
it will be easier to run the commands only for Volto core within the Volto core folder: | ||
|
||
```shell | ||
cd packages/volto | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should mention the |
||
``` | ||
|
||
From here we will have access to the commands to check for errors and to fix them as well, and we will go through all of the previously enumerated commands in the following section | ||
|
||
### Eslint, Prettier and Stylint | ||
|
||
You can run the `eslint`, `prettier` and `stylelint` commands by running the following from the Volto package folder: | ||
|
||
```shell | ||
pnpm lint | ||
pnpm prettier | ||
pnpm stylelint | ||
``` | ||
|
||
If we get any errors some of them can be solved automatically by running `"lint-command":fix`: | ||
|
||
```shell | ||
pnpm lint:fix | ||
pnpm prettier:fix | ||
pnpm stylelint:fix | ||
``` | ||
|
||
> **Note**: | ||
The same commands can be found also in your Volto add-on projects as seen in the [package.json.tpl](https://github.com/plone/volto/blob/main/packages/generator-volto/generators/app/templates/package.json.tpl#L10) file. | ||
> | ||
> You will use similar commands to run the linting commands using `yarn`: | ||
|
||
```shell | ||
yarn lint | ||
yarn lint:fix | ||
yarn prettier | ||
yarn prettier:fix | ||
yarn stylelint | ||
yarn stylelint:fix | ||
``` | ||
|
||
If the fix commands cannot fix the errors given by the linting commands you will need to fix the errors manually otherwise the commit will fail the testing infrastructure. If that happens your contribution will not be able to be merged until the pull request has every test passing. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Updated testing and code quality with information on how to contribute to Volto core. @ichim-david |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true. You can run them from the root too, they have similar proxy commands to
packages/volto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because you can do something doesn't mean you should do it. We are documenting to beginners how they can run the acceptance testing and if they really care they can see that the commands expand to the volto package.
Within the volto package you have everything you need from the news folder to lint to run to test and although power users can do whatever they want from volto repo, well that's a power user move :).
Although I didn't write it this way exactly I agree with Steve's directive to assume all commands for volto core run from packages/volto folder.
EDIT:
Another reason to use the workflow of go inside packages/volto folder is that we can show the commands using only pnpm command which would have the equivalent also in projects and 17.x.x and 16.x.x branch by swapping pnpm for yarn instead of going over
pnpm --filter which doesn't have equivalent in the order, still maintain and I would even say current versions until 18 is released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually didn't recommend one place over another. It was my interpretation of the link that @ichim-david had originally used to refer to this location. This brings up something we need to discuss and decide.
There are quite a lot of differences between the two
Makefile
s. It looks like the primaryMakefile
is at the repo root, based in its inclusion of documentation commands and reading files from the repo root. It also appears that the one inpackages/volto
has not been kept up to date with that at the repo root, based on the number of updates to each. For example, I would not runmake build
from the package, but I would from the root. Here's another significant difference where some commands fail when spaces are in therealpath
, which is something that often bites first-timers on their bum.After looking at these differences as they are today, I think that we should recommend all commands should be run from the root of the repo. We can also suggest a tip that they may be run from the
packages/volto
directory, but there is no guarantee that they will work, such asmake docs-*
. That is, unless and until, we decide that theMakefile
in thepackages/volto
directory is maintained for full parity with that at the root of the repo.@sneridagh is that interpretation of
Makefile
s correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, https://github.com/plone/volto/blob/main/package.json#L14-L23 although yes in the root you have commands for lint, prettier and stylelint and the :fix options they run for all of the apps and packages defined in the monorepo while the commands from packages/volto only run for the volto core package.
Yes, you can also manually run the filter commands from the root but then you lose the convenience of simply writing pnpm command and getting the right outcome.
We can enhance the docs with another section for the apps and other packages from the volto umbrella but now we should ensure that we have the docs for how to contribute to Volto core in my opinion.