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.

Sign up for GitHub

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

Jump to bottom

CI - Add code sniffer #744

Merged
merged 3 commits into from
Sep 18, 2024
Merged

CI - Add code sniffer #744

merged 3 commits into from
Sep 18, 2024

Conversation

timotei-litespeed
Copy link
Contributor

No description provided.

@timotei-litespeed timotei-litespeed mentioned this pull request Sep 16, 2024
Closed
tynanbe
tynanbe suggested changes Sep 17, 2024
View reviewed changes
.github/workflows/ci.yml Outdated
# concurrency:
# group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }}
# cancel-in-progress: true

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove L11-15 completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

.github/workflows/ci.yml Outdated
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

# Initialize Cache Composer
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

.github/workflows/ci.yml Outdated
path: .cache
key: ${{ runner.os }}-${{ hashFiles('**/composer.lock') }}

# Initialize Node.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this comment the step's name instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to name

.github/workflows/ci.yml Outdated

- run: npm ci
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, change ci to clean-install

Copy link
Contributor Author

@timotei-litespeed timotei-litespeed Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should I do something here?

Copy link
Contributor

Choose a reason for hiding this comment

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

      - run: npm clean-install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

.prettierrc Outdated
@@ -2,14 +2,20 @@
"overrides": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all .prettierrc changes from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to previous version

Copy link
Contributor

Choose a reason for hiding this comment

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

The file shouldn't show up at all in https://github.com/litespeedtech/lscache_wp/pull/744/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

composer.json Outdated
"scripts": {
"post-install-cmd": "phpcs --config-set installed_paths vendor/phpcompatibility/php-compatibility",
"post-update-cmd": "phpcs --config-set installed_paths vendor/phpcompatibility/php-compatibility",
"sniff-check": "phpcs --standard=./phpcs.xml.dist"
Copy link
Contributor

Choose a reason for hiding this comment

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

PHPCS should find the "standard" config without specifying it here. Please, test and confirm this works.

"sniff-check": "phpcs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works and changed

@hi-hai hi-hai merged commit a78c408 into litespeedtech:v7.0 Sep 18, 2024
1 check failed
@timotei-litespeed
Copy link
Contributor Author

@szepeviktor these changes have been applied to the future version of the plugin.
Talking to my colleagues we decided to look into new ways to improve error detection, improve the security and help maintain the plugin easier.
If you're still up for it: what suggestions do you have? Please create a branch from https://github.com/litespeedtech/lscache_wp/tree/v7.0 and add changes into a PR.

Thank you!

@szepeviktor
Copy link
Contributor

szepeviktor commented Sep 18, 2024 •
edited
Loading

what suggestions do you have?

#742 (comment)

Please consider choosing a coding standard: PSR, WordPress, custom or a minimal one.
This is my custom ruleset: https://github.com/szepeviktor/phpcs-psr-12-neutron-hybrid-ruleset

I could add EditorConfig (for Prettier) and syntax error check.

As for static analysis: the plugin should pass PHPStan level 0.

@timotei-litespeed timotei-litespeed deleted the CI-add_code_sniffer_final branch September 19, 2024 12:20
@timotei-litespeed timotei-litespeed restored the CI-add_code_sniffer_final branch September 19, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tynanbe tynanbe tynanbe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timotei-litespeed @szepeviktor @tynanbe @hi-hai @Tymotey