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

Test enhancement #84

Merged

Conversation

peter279k
Copy link
Contributor

Q A
Bug fix? no
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT

Changed log

  • Add php-7.2 and php-7.3 version tests on Travis CI build.
  • Using the ::class magic class string call to replace the class name string.
  • Removing unused variables.
  • Using the assertCount to assert the expected count value is same as result count value.
  • According to the PHPUnit fixture reference, the setUp method is protected, not public.

When using the composer --prefer-lowest and --prefer-stable flags on php-7.2 version, this will cause the following coding style issue:

 php vendor/bin/phpcs                           19.5s
............................................E 45 / 45 (100%)



FILE: src/MetadataFactory.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 36 | ERROR | Class MetadataFactory contains write-only property
    |       | $hierarchyMetadataClass.
    |       | (SlevomatCodingStandard.Classes.UnusedPrivateElements.WriteOnlyProperty)
--------------------------------------------------------------------------------

Time: 804ms; Memory: 8Mb

It seems that the private $hierarchyMetadataClass variable is unused.

To pass this coding style check, removing this variable.

Copy link
Collaborator

@goetas goetas left a comment

Choose a reason for hiding this comment

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

Hi,

thanks for the PR. Looks good. I've noticed only one thing to be changed.

/**
* @var null|string
*/
private $hierarchyMetadataClass;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this property should not have been removed

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 remove this variable because the coding style check will be failed.

Here is the history log.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is the bug of the style checker, the property is being read at

$metadata = new $this->hierarchyMetadataClass();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Reverting this change :).

@goetas
Copy link
Collaborator

goetas commented Oct 24, 2019

To not have https://travis-ci.org/open-source-contributions/metadata/jobs/602205210 failing, you can assign hierarchyMetadataClass to a temporary local variable in

$metadata = new $this->hierarchyMetadataClass();

like:

$class = $this->hierarchyMetadataClass;
$metadata = new $class();

That is even more correct than the current code.

@peter279k
Copy link
Contributor Author

$this->hierarchyMetadataClass;

Thanks for your reply. But it still has the same coding style issue.

@peter279k
Copy link
Contributor Author

$this->hierarchyMetadataClass;

Thanks for your reply. But it still has the same coding style issue.

Ok. I got no coding style error finally. Forgiving my fault 😄.

@goetas
Copy link
Collaborator

goetas commented Oct 24, 2019

👍

@goetas
Copy link
Collaborator

goetas commented Oct 24, 2019

I think that there is an error in your rebase... I can see some extra commits

@peter279k
Copy link
Contributor Author

peter279k commented Oct 24, 2019

I think that there is an error in your rebase... I can see some extra commits

Sorry. I make mistake about rebase...

Reverting commits.

@goetas goetas merged commit 8692edc into schmittjoh:master Oct 24, 2019
@goetas
Copy link
Collaborator

goetas commented Oct 24, 2019

Thank you

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.

2 participants