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

Phpcs Baseline testing #24

Closed
wants to merge 231 commits into from
Closed

Conversation

MeetKamal
Copy link
Contributor

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@MeetKamal MeetKamal requested a review from a team as a code owner April 3, 2023 14:00
Copy link
Contributor

@sprankhub sprankhub left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I hope my comments make sense to you :)

- name: Run Coding Standard
uses: graycoreio/github-actions-magento2/coding-standard@main
- uses: actions/checkout@v3
- uses: './coding-standard'
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we should have a new directory coding-standard in the root directory. IMHO, the action should be included here directly. So the steps from coding-standard/action.yml can simply be included here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have tried without that coding-standard folder, when we try to get input variables without that folder, its not getting those values. So we have put that as it is for now. Please review

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct way would be to move the action to https://github.com/mage-os/infrastructure and just call it here similar to the PHP Unit Test action being prepared in mage-os/infrastructure#34.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, will discuss on this

Copy link
Contributor

@Vinai Vinai May 16, 2023

Choose a reason for hiding this comment

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

I agree with @sprankhub, we don't want a new folder in the project root for CI purposes. I know it is not necessary, the input variables do not depend on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming, @Vinai. Then this PR needs to be split up into one PR to https://github.com/mage-os/infrastructure and one PR into this repo here.

coding-standard/action.yml Outdated Show resolved Hide resolved
coding-standard/action.yml Outdated Show resolved Hide resolved
id: phpcs_before
working-directory: standard
run: |
grep -i "\.php$" ${{ github.workspace }}/file_lists_old.txt > ${{ github.workspace }}/file_php_old.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

I think .php is not enough. We also need to add .phtml and maybe more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have added .phtml now, which others are required to add ? phpcs only works for php code base.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like more files are checked by phpcs - did you have a look?

coding-standard/action.yml Outdated Show resolved Hide resolved
coding-standard/action.yml Outdated Show resolved Hide resolved
coding-standard/action.yml Outdated Show resolved Hide resolved
@sprankhub sprankhub closed this Oct 5, 2023
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