Skip to content
This repository was archived by the owner on Apr 2, 2024. It is now read-only.

Conversation

@andriyun
Copy link
Contributor

Part of https://os2web.atlassian.net/browse/OS2FORMS-380

@andriyun andriyun requested a review from rimi-itk June 21, 2022 07:25
Copy link
Collaborator

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

Looks good. Some comments on coding standards tools added.

Comment on lines 24 to 25
composer require dealerdirect/phpcodesniffer-composer-installer --no-interaction
composer require drupal/coder --no-interaction
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should add these to require-dev in composer.json to make it easy to run the checks locally before committing code.

We could also borrow from https://github.com/itk-dev/os2forms_selvbetjening/blob/develop/composer.json#L92-L95 (to also include static code analysis if we dare) and add some helper scripts to make it even easier to run the checks: https://github.com/itk-dev/os2forms_selvbetjening/blob/develop/composer.json#L197-L224

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rimi-itk thank you for feedback :)

I've added dealerdirect/phpcodesniffer-composer-installer and drupal/coder to as dev dependencies.

Including static code, analysis requires establishing Drupal project structure and injecting the subject repository into it.
IMO it makes more sense to have it on installation like https://github.com/itk-dev/os2forms_selvbetjening

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, running code analysis on a Drupal module in a repository is tricky, @andriyun, but it can be done.

We have done it in https://github.com/itk-dev/azure_ad_delta_sync/blob/develop/.github/workflows/pr.yaml#L95-L113, but let's get back to that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to go with the approach you proposed. It looks like it's not so easy for this case.
This kind of check was not part of this task in the beginning. So I'd rather move it out of scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. We should just stick with coding standards for now.

Copy link
Collaborator

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

Please apply coding standards.

@andriyun andriyun force-pushed the OS2FORMS-380-github-actions branch 4 times, most recently from 065ae7a to dd57e53 Compare June 21, 2022 15:48
@andriyun
Copy link
Contributor Author

andriyun commented Jun 21, 2022

@rimi-itk

Please apply coding standards.

Do you mean fix warnings and errors in PHP code sniffer report?

@rimi-itk
Copy link
Collaborator

Yes, @andriyun, all checks should succeed in this merge request. Otherwise the checks don't add any value.

@andriyun andriyun force-pushed the OS2FORMS-380-github-actions branch from 4eac31d to 0b239ab Compare June 22, 2022 14:33
@andriyun andriyun force-pushed the OS2FORMS-380-github-actions branch from 0b239ab to 132d14d Compare June 22, 2022 14:56
@andriyun
Copy link
Contributor Author

@rimi-itk I've fixed coding standards issues

@andriyun andriyun requested a review from rimi-itk June 22, 2022 15:01
@rimi-itk
Copy link
Collaborator

Excellent work, @andriyun!

@perthaarhus perthaarhus merged commit 835e515 into develop Jun 27, 2022
@perthaarhus perthaarhus deleted the OS2FORMS-380-github-actions branch June 27, 2022 08:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants