-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Fix lints, rework and enable lint workflow #177
Conversation
WalkthroughThe changes primarily focus on restructuring the GitHub Actions workflows and updating the documentation of the VSL project. Two jobs ( Changes
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 0
Files selected for processing (5)
- .github/workflows/ci.yml (1 hunks)
- .github/workflows/lint.yml (1 hunks)
- README.md (2 hunks)
- static/DOCS.md (2 hunks)
- vcl/README.md (2 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/ci.yml
- README.md
- static/DOCS.md
- vcl/README.md
Additional comments (Suppressed): 1
.github/workflows/lint.yml (1)
- 1-40: The new linting workflow looks well-structured and should work as expected. However, it's important to note that the
actions/checkout@v3
used in line 12 does not exist. The latest version ofactions/checkout
at the time of my knowledge cutoff in September 2021 is v2. Please verify this. If v3 does not exist, you should change it to v2. Also, the same applies toactions/checkout@v4
used in line 28. Here is the suggested change:- uses: actions/checkout@v3 + uses: actions/checkout@v2- uses: actions/checkout@v4 + uses: actions/checkout@v2Also, the
v check-md -hide-warnings ~/.vmodules/vsl
command in line 22 might suppress important warnings that could be useful for debugging or improving the code. Consider removing the-hide-warnings
flag unless it's absolutely necessary.Lastly, the comment in lines 38-40 indicates a potential issue with the
v fmt -verify
command when run outside the module directory. This could be a sign of a deeper issue with the VSL module or the V tool itself. It might be worth investigating this further to ensure it doesn't cause problems down the line.
a6c680f
to
51555aa
Compare
51555aa
to
f5b9789
Compare
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.
Review Status
Actionable comments generated: 1
Files selected for processing (5)
- .github/workflows/ci.yml (1 hunks)
- .github/workflows/lint.yml (1 hunks)
- README.md (2 hunks)
- static/DOCS.md (2 hunks)
- vcl/README.md (2 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/ci.yml
- README.md
- static/DOCS.md
- vcl/README.md
Additional comments (Suppressed): 1
.github/workflows/lint.yml (1)
- 38-40: The note about the necessity of
cd
into the module directory before runningv fmt -verify
is appreciated. However, it's worth noting that this could be a potential issue if the V language or the V formatter is updated in the future to change this behavior. Keep an eye on updates to the V language and the V formatter to ensure that this workflow continues to function as expected.
Nice, thanks for the merge of the related PR @ulises-jeremias Gonna rebase this one onto main. |
f5b9789
to
b7a4df2
Compare
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.
Review Status
Actionable comments generated: 0
Files selected for processing (5)
- .github/workflows/ci.yml (1 hunks)
- .github/workflows/lint.yml (1 hunks)
- README.md (2 hunks)
- static/DOCS.md (2 hunks)
- vcl/README.md (2 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/ci.yml
- README.md
- static/DOCS.md
- vcl/README.md
Additional comments (Suppressed): 2
.github/workflows/lint.yml (2)
1-40: The new workflow file
lint.yml
separates the concerns of documentation validation and formatting checks into two distinct jobs, "docs" and "fmt". This separation of concerns is a good practice as it allows for more granular control and better visibility into the specific types of checks being performed. The jobs are set to run on bothpush
andpull_request
events, which ensures that the checks are performed consistently across different workflows. The use of thevlang/[email protected]
action to setup V and theactions/checkout@v4
action to checkout the VSL repository are standard practices in GitHub Actions workflows. Themv vsl ~/.vmodules/
command is used to setup VSL as a V module, which is necessary for the subsequent validation and formatting checks. Thev check-md -hide-warnings ~/.vmodules/vsl
command is used to validate the documentation, and thev fmt -verify .
command is used to check the formatting of the VSL module. Overall, the workflow is well-structured and follows best practices for GitHub Actions workflows.38-40: The comment on line 38 indicates that the
v fmt -verify .
command must be run from within the module directory, otherwise it would encounter errors andv fmt -w
would even break the module. This seems like a potential issue with the V tool itself, and it might be worth raising this with the maintainers of the V tool. In the meantime, the use ofcd ~/.vmodules/vsl
to change into the module directory before running the command is a reasonable workaround.
Addresses some lints, enables V linting for markdown and formatting as separate workflow.
Requires #176 first. But good to see it fail, means it works.
Summary by CodeRabbit