Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Add code linting and github action #73

Merged
merged 12 commits into from
Oct 11, 2021
Merged

Add code linting and github action #73

merged 12 commits into from
Oct 11, 2021

Conversation

jffng
Copy link
Collaborator

@jffng jffng commented Oct 8, 2021

Description

This PR is a simpler alternative to #26. (Does not include the pre-commit hook)

Testing Instructions

  1. Run npm install && composer install
  2. Try all of the test commands:
  • npm run lint:css
  • composer run analyze functions.php
  • composer run lint
  • composer run standards:check
  • composer run standards:fix

pull_request:
branches: [trunk]
push:
branches: [trunk,add/linting-github-actions]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should remove this branch from the array before merging.

@jffng
Copy link
Collaborator Author

jffng commented Oct 8, 2021

Also @EvanHerman if you have a chance to review that would be great, thank you.

- name: install node v12
uses: actions/setup-node@v1
with:
node-version: 12
Copy link
Contributor

@gregrickaby gregrickaby Oct 8, 2021

Choose a reason for hiding this comment

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

@jffng I'm a big fan of Github Actions. Your workflows look really great!

Question about the Node version though. The current LTS is 14 (which has support well into 2023) in contrast, Node 12 has an end-of-life date of 2022-04-30.

Thoughts about using Node 14 LTS instead?

Source: https://nodejs.org/en/about/releases/

Copy link
Member

Choose a reason for hiding this comment

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

WordPress & Gutenberg's _official support is "LTS"

That said Gutenberg is still running Node.js v12 & v14 tests in GitHub Actions

Stylelint in the upcoming v14 release drops Node.js v10 support

With all that said, there's probably no need to use v12, switching to v14 should be fine

uses: actions-hub/stylelint@master
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
PATTERN: "**/*.css"
Copy link
Contributor

Choose a reason for hiding this comment

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

@jffng Just passing along @ntwb comment that all files will need a blank new line. There are a few in this PR which are missing them.

Github flags them with a handy icon:
screenshot

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Looking good @jffng, a few more minor'ish suggestions

.editorconfig Outdated Show resolved Hide resolved
steps:
# Checkout repository
- name: Checkout
uses: actions/checkout@v2
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest pinning these versions for all GitHub Actions, it ensures that only explicit versions are used, this is primarily I believe security related and is adhered to in the majority of P projects, e.g Gutenberg here for example:

https://github.com/WordPress/gutenberg/blob/trunk/.github/workflows/static-checks.yml#L25

These can be kept up-to-date with Dependabot:

https://docs.github.com/en/code-security/supply-chain-security/keeping-your-dependencies-updated-automatically/keeping-your-actions-up-to-date-with-dependabot

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/stylelint.yml Outdated Show resolved Hide resolved
run: |
echo "::set-output name=dir::$(composer config cache-files-dir)"
- name: Cache composer dependencies
uses: actions/cache@v1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: actions/cache@v1
uses: actions/cache@c64c572235d810460d0d6876e9c705ad5002b353 # v2.1.6

Updates v1 to the latest v2 release https://github.com/actions/cache/releases/tag/v2.1.6

run: |
echo "::set-output name=dir::$(composer config cache-files-dir)"
- name: Cache composer dependencies
uses: actions/cache@v1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: actions/cache@v1
uses: actions/cache@c64c572235d810460d0d6876e9c705ad5002b353 # v2.1.6

Updates v1 to the latest v2 release https://github.com/actions/cache/releases/tag/v2.1.6

steps:
- uses: actions/checkout@v2
- name: install node v12
uses: actions/setup-node@v1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: actions/setup-node@v1
uses: actions/setup-node@270253e841af726300e85d718a5f606959b2903c # v2.4.1

Upgrades v1 to the latest setup-node v2.4.1 release https://github.com/actions/setup-node/releases/tag/v2.4.1

@@ -0,0 +1,32 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This file has mixed indentation, currently using a mix of both tabs and spaces, it should use only tabs, this inherits the same settings from the [*] .editorconfig file pattern

The dots in this indicates spaces, arrows indicate tabs:
image

@@ -0,0 +1,23 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This file uses spaces for, it should use tabs, JSON files inherit the same settings from the [*] .editorconfig file pattern

@jffng
Copy link
Collaborator Author

jffng commented Oct 11, 2021

@ntwb @gregrickaby thank for the reviews and feedback! I think I addressed it all, including:

  • Pinning gh action dependencies to specific commits / versions
  • Using node 14 for the stylelint action
  • Adding newlines to files that needed them
  • Naming / formatting improvements

Going to merge this, thanks again.

@jffng jffng merged commit 6896110 into trunk Oct 11, 2021
@jffng jffng deleted the add/linting-github-actions branch October 11, 2021 14:22
@ntwb
Copy link
Member

ntwb commented Oct 12, 2021

@jffng Unsure if stylelint is actually working, it's not clear at all looking at the action run:

https://github.com/WordPress/twentytwentytwo/runs/3860275082?check_suite_focus=true

Run actions-hub/stylelint@master
  env:
    GITHUB_TOKEN: ***
    PATTERN: **/*.css
/usr/bin/docker run --name fa4e1462043903626f4732ae6d2265fbc0d97a_c97241 --label fa4e14 --workdir /github/workspace --rm -e GITHUB_TOKEN -e PATTERN -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_RUN_ATTEMPT -e GITHUB_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e RUNNER_OS -e RUNNER_NAME -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "/home/runner/work/_temp/_github_home":"/github/home" -v "/home/runner/work/_temp/_github_workflow":"/github/workflow" -v "/home/runner/work/_temp/_runner_file_commands":"/github/file_commands" -v "/home/runner/work/twentytwentytwo/twentytwentytwo":"/github/workspace" fa4e14:62043903626f4732ae6d2265fbc0d97a
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
warning "stylelint" is already in "devDependencies". Please remove existing entry first before adding it to "dependencies".
**/*.css

Also, the PHPCS run doesn't warn of the warnings:

https://github.com/WordPress/twentytwentytwo/runs/3860275079?check_suite_focus=true

Run composer standards:check -- --runtime-set ignore_warnings_on_exit true --runtime-set testVersion 5.8-
> @php ./vendor/squizlabs/php_codesniffer/bin/phpcs '--runtime-set' 'ignore_warnings_on_exit' 'true' '--runtime-set' 'testVersion' '5.8-'
W. 2 / 2 (100%)



FILE: index.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | WARNING | No PHP code was found in this file and short open tags
   |         | are not allowed by this install of PHP. This file may
   |         | be using short open tags but PHP does not allow
   |         | them. (Internal.NoCodeFound)
----------------------------------------------------------------------

Time: 159ms; Memory: 6MB

jeffpaul added a commit to jeffpaul/twentytwentytwo that referenced this pull request Nov 16, 2021
Note that some of the references below are not the only contributions (commits, PR review, helpful issue input, etc.) from these folks, but merely the first ones I encountered while reviewing items committed to `trunk` since the initial branch commit.

@westonruter via  WordPress#51
@ntwb via WordPress#73
@juricav via WordPress#113
@Sandstromer via WordPress#69
@jasmussen via WordPress#74
@melchoyce via WordPress#16
@Riyadh1734 via WordPress#182
@desrosj via WordPress#223
@beafialho via WordPress#172
@clucasrowlands via WordPress#171
@Otto42 via WordPress#28
@luminuu via WordPress#107
@felixarntz via WordPress#240
kjellr added a commit that referenced this pull request Nov 17, 2021
* add missing props to CONTRIBUTORS.md

Note that some of the references below are not the only contributions (commits, PR review, helpful issue input, etc.) from these folks, but merely the first ones I encountered while reviewing items committed to `trunk` since the initial branch commit.

@westonruter via  #51
@ntwb via #73
@juricav via #113
@Sandstromer via #69
@jasmussen via #74
@melchoyce via #16
@Riyadh1734 via #182
@desrosj via #223
@beafialho via #172
@clucasrowlands via #171
@Otto42 via #28
@luminuu via #107
@felixarntz via #240

* add dotorg handles to CONTRIBUTORS.md

* Fix Rich's WP.org username.

Co-authored-by: Kjell Reigstad <kjell@kjellr.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants