Skip to content

implemented unit tests, added phpcs, static-analysis and run unit tes…#15

Merged
arhimede merged 6 commits intodotkernel:2.0from
poprazvan17:2.0
Jul 18, 2023
Merged

implemented unit tests, added phpcs, static-analysis and run unit tes…#15
arhimede merged 6 commits intodotkernel:2.0from
poprazvan17:2.0

Conversation

@poprazvan17
Copy link
Copy Markdown
Collaborator

…t github actions

Copy link
Copy Markdown
Member

@alexmerlin alexmerlin left a comment

Choose a reason for hiding this comment

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

Please fix the following items:

  1. Make sure there is an empty line at the end of each the following files:
  • .github/workflows/cs-tests.yml
  • .github/workflows/static-analysis.yml
  • .github/workflows/unit-tests.yml
  • phpcs.xml
  • phpunit.xml
  • psalm.xml
    In case that they already have an empty line at the end, make sure the line endings are set to LF (NOT CRLF).
  1. All file header blocks should be removed:

/**

  1. Make sure that the below commands report no error:
  • composer cs-check
  • composer static-analysis
  • composer test

Comment thread composer.json Outdated
Comment thread composer.json
Comment thread composer.json
Comment thread src/AuthenticationResult.php
Comment thread tests/AuthenticationResultTest.php Outdated
Comment thread composer.json Outdated
@alexmerlin
Copy link
Copy Markdown
Member

Also, please add Symfony Insight badge.
If you don't have the snippet for it, ask @arhimede to create it.

@alexmerlin alexmerlin linked an issue Jul 11, 2023 that may be closed by this pull request
Copy link
Copy Markdown
Member

@alexmerlin alexmerlin left a comment

Choose a reason for hiding this comment

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

@poprazvan17 A couple of my comments still aren't resolved (you can also see them above, not being marked as resolved).
I'll summarize:

  1. for consistency across our packages, please move test file back to directory test (make sure you change the directory name, in composer.json, phpcs.xml and in phpunit.xml)
  2. please add Symfony Insight badge. If you don't have the snippet for it, ask @arhimede to create it.
  3. see my unresolved comments across the PR

Comment thread composer.json Outdated
Comment thread composer.json Outdated
Comment thread src/Identity/IdentityInterface.php Outdated
@alexmerlin
Copy link
Copy Markdown
Member

2 observations:

  1. composer.json: The whole file appears as modified. Did you modify line endings from CRLF to LF? Or vice-versa? (being a fork PR, I cannot see)
  2. don't forget to add Symfony Insight badge

Other than these, LGTM.

@poprazvan17
Copy link
Copy Markdown
Collaborator Author

poprazvan17 commented Jul 13, 2023 via email

Copy link
Copy Markdown
Member

@alexmerlin alexmerlin left a comment

Choose a reason for hiding this comment

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

@poprazvan17 Please insert a new line between the Github license and the SymfonyInsight badge, so the SymfonyInsight badge is rendered on a new line.

Copy link
Copy Markdown
Member

@alexmerlin alexmerlin left a comment

Choose a reason for hiding this comment

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

@poprazvan17 Also, I see that composer.json is CRLF, please convert it to LF.
SymfonyInsight also reports an incorrect end of file.

@arhimede arhimede merged commit 6bd9bfa into dotkernel:2.0 Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Code quality

4 participants