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

Config: add tests for the --generator= argument #765

Merged

Conversation

rodrigoprimo
Copy link
Contributor

Description

In preparation for a PR to address #709, this PR adds tests to cover the current behavior of the code in the Config class that handles setting the generator property when the --generator= argument is passed in the command line.

Related issues/external references

Related to #709

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

Copy link
Member

@jrfnl jrfnl 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 this @rodrigoprimo .

I presume a test with the argument being passed an invalid value will follow in the PR to address #709 ?

I've left some small nitpicks inline. Also noticed the @dataProvider and @see tags being at the end of the docblocks, which is not consistent with their use elsewhere, but not blocking (as we don't enforce a specific order, which I would love to do, but that will need to wait for a proper Docs standard).

tests/Core/Config/GeneratorArgTest.php Outdated Show resolved Hide resolved
tests/Core/Config/GeneratorArgTest.php Outdated Show resolved Hide resolved
tests/Core/Config/GeneratorArgTest.php Outdated Show resolved Hide resolved
@jrfnl jrfnl added this to the 3.11.x milestone Dec 11, 2024
@rodrigoprimo
Copy link
Contributor Author

Thanks for the review, @jrfnl!

I presume a test with the argument being passed an invalid value will follow in the PR to address #709 ?

Yes, I opted not to include it, as the code does not currently check for invalid values. But now I see that I could have included it to document the code's current behavior and then update the test in the subsequent PR.

Also noticed the @dataProvider and @see tags being at the end of the docblocks, which is not consistent with their use elsewhere, but not blocking

I used tests/Core/Config/SniffsExcludeArgsTest.php as a reference to create this test, and it also has the @dataProvider tags below the @return tags. That being said, it is something I overlooked and not something I think we should do. So, I went ahead and updated this test file to fix the placement of the @dataProvider and @see tags.

This PR is ready for another review.

@jrfnl
Copy link
Member

jrfnl commented Dec 12, 2024

I presume a test with the argument being passed an invalid value will follow in the PR to address #709 ?

Yes, I opted not to include it, as the code does not currently check for invalid values. But now I see that I could have included it to document the code's current behavior and then update the test in the subsequent PR.

Do you still want to do that or would you prefer for this PR to be merged and the next one to document handling of invalid values ?

@rodrigoprimo
Copy link
Contributor Author

Do you still want to do that or would you prefer for this PR to be merged and the next one to document handling of invalid values ?

The easiest for me would be to have this PR merged and include the other test in the subsequent PR. But I'm happy to update this PR if you prefer.

@jrfnl jrfnl merged commit 3d14d00 into PHPCSStandards:master Dec 12, 2024
45 checks passed
@jrfnl jrfnl deleted the test-config-generator-argument branch December 12, 2024 20:43
@jrfnl
Copy link
Member

jrfnl commented Dec 12, 2024

Okay, done. Looking forward to the follow-up PR.

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.

2 participants