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

CI changes #742

Closed

Conversation

timotei-litespeed
Copy link
Contributor

Added PHP Code sniffer integration

For local:

  • in Visual code install plugin: PHP Sniffer
  • Plugin settings:
    • Auto detect set to Checked
    • Optional: can be set to run on save

Prerequirement: Node 16, Composer, PHP 7.2
Install:
make sure there is a empty folder: .cache in root of plugin folder
open terminal
run: npm install
run: composer install
to run sniffer manually: composer run sniff-check

Hai Zheng and others added 30 commits September 6, 2024 13:30
…2.0. * **Core** Minimum required WP version escalated to WP v5.3.
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
"./vendor/bin/phpcs -p . --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.

There should be a newline at end of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It get removed by Prettier. Should I add it manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that's fine.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
@szepeviktor
Copy link
Contributor

I think this is not the GHA setup you want.
May I contribute an improved strategy after this one gets merged?

@timotei-litespeed
Copy link
Contributor Author

timotei-litespeed commented Sep 13, 2024

Contribute now, before merge

@szepeviktor
Copy link
Contributor

Contribute now, before merge

I have to inform you that I am struggling with

  • lack of EditorConfig (for Prettier)
  • lack of syntax error check
  • lack of coding standard
  • lack of static analysis
  • all right, we don't have unit/E2E tests

@timotei-litespeed
Copy link
Contributor Author

@szepeviktor
Ok, No problem.
Do you have any suggested changes to Github CI actions?

For us current script is almost enought, we added Code Sniffer as a code check, no to correct or fix the code.

@szepeviktor
Copy link
Contributor

Do you have any suggested changes to Github CI actions?

Yes. All the above tools. e.g. #696

Add Code Sniffer actions
@timotei-litespeed
Copy link
Contributor Author

Moved to #744

@szepeviktor thank you for the code and iddeeas
We are not really using WP code style and we are not adding Code Beautify(at least for now there are no plans)

@szepeviktor
Copy link
Contributor

szepeviktor commented Sep 16, 2024

@timotei-litespeed Without the above tools, or without a coding standard you are theoretically cut off from bug-free code. That is very sad to see.

We are not really using WP code style

You could have a custom coding standard!!

@timotei-litespeed
Copy link
Contributor Author

I agree that withput a coding standard there is posibility for bugs to appear. For now this automation will help us make the code cleaner.

Thank you for suggestions but for NOW the changes are enought for the time allocated for this change.

👍

@Tymotey Tymotey deleted the CI-add_code_sniff branch September 19, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants