Improve code quality #38

tux-rampage · 2018-08-29T11:53:05Z

This PR intends to provide the following QA improvements:

Increase Test-Coverage (Missing tests)
Make resolved type/value injections immutable
- Setters/Getters are not needed, the whole property name is not needed at all
- Provide toValue() to obtain the value to inject
Create an interface for type/value injection to get rid of too complex instanceof checks
Generate PHP 7.1+ code

I will add comments to the diff to clarify my indentations

Is this related to quality assurance?

TODOs:

Update documentation
Add CHANGELOG.md entries

An interface allows more flexibility to implementers/consumers. The setter/getter for parameter name is polluting the instance's state and this information is not really needed. It's the job of the resolver to provide the name/injection link.

Ocramius · 2018-08-29T11:57:39Z

src/Resolver/AbstractInjection.php

@@ -20,6 +28,11 @@
     */
    public function setParameterName(string $name) : self
    {
+        trigger_error(


To be documented in CHANGELOG.md

Cahngelog updates will follow. this is still WIP

src/Resolver/AbstractInjection.php

Ocramius · 2018-08-29T11:58:17Z

src/Resolver/InjectionInterface.php

+    public function export() : string;
+
+    /**
+     * @return bool


src/Resolver/TypeInjection.php

src/Resolver/ValueInjection.php

Ocramius · 2018-08-29T11:59:56Z

test/CodeGenerator/FactoryProphecy.php

+use Zend\Di\CodeGenerator\FactoryInterface;
+use ZendTest\Di\TestAsset\InvokableInterface;
+
+final class FactoryProphecy implements FactoryInterface


Urgh, would be better to avoid prophecy at all if it needs to become this complex to please the tool: why do traditional mocks not work here?

Yeah, I don't like it either. Not sure how to replace it. The scenario:

Expect that if the $factories[$key] value is a string, this string is treated as class name and instanciated. But maybe I'm thinking too complex here.

This may be time to get rid of the zend-code dependency

PHP will export NULL, which conflicts with the ZF coding standard.

tux-rampage · 2018-08-29T13:35:22Z

test/_files/expected-factories/with-params.php

+use function is_array;
+
+final class MixedArgumentsFactory implements FactoryInterface
+{


This is how the output should finally look like. To achieve this I'd remove zend-code. This simplifies the generator, docs and consumers do no longer need zend-code for AoT

test/CodeGenerator/AbstractInjectorTest.php

Adding a strict return type and throw exceptions on failure makes the implementation predictable.

CHANGELOG.md

This will not trigger a warning as file_put_contents would. Instead it will throw when opening the file for read access fails.

froschdesign · 2018-09-03T10:02:26Z

src/CodeGenerator/AutoloadGenerator.php

    public function __construct(string $namespace)
    {
        $this->namespace = $namespace;
    }

-    private function generateAutoloaderClass(array &$classmap)
+    private function writeFile(string $filename, string $code): void


Only a short notice: space before colon is missing.

Strange, why didn't the cs-check complain about it? Maybe an outdated version. Thanks for pointing this out. 👍

@froschdesign I ran composer update. The locked version of zend-coding-standard was not out of date. cs-check is still not complaining. Worth a bug report on zendframework/zend-coding-standard? For the sake of consistency I'd argue to leave it this way for the moment and fix this in a separate pull request. Is this OK for you?
(edit: fix link)

tux-rampage · 2018-10-19T14:11:30Z

I've dismissed some minor CS issues that will be handled once ZF CS is applied. Only thing missing here is the documentation of the newly introduced interface

Thanks a lot, @Ocramius

Do you mean the InjectionInterface: https://github.com/zendframework/zend-di/pull/38/files#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR17 ?

What kind of documentation do you have in mind? This is mostly internal and only relevant when somebody wants to implement a custom Resolver. It's not a common use case.

Ocramius · 2018-10-19T14:33:30Z

@tux-rampage even if it is internal, so then it should:

be marked as @internal
documented for other maintainers: purpose, behavior, expected usage and expected usage locations

tux-rampage · 2018-10-19T18:29:59Z

@tux-rampage even if it is internal, so then it should:

be marked as @internal

documented for other maintainers: purpose, behavior, expected usage and expected usage locations

@Ocramius, sorry if I expressed this incorrectly, my comment was not about if this should be documented. It was more a question about how.

I hesitate marking this internal. Even though implementing a custom resolver is a rare use case I'd like to give users the freedom with an explicit contract when they know what they're doing. That's the reason why it's not covered in the doc book, yet: It's not something you need to do to get zend-di working and it's something you should consider twice before doing it (i.e. it's better to write a service factory instead of putting additional magic into the auto wiring implementation).

I added some additional PHPDoc. Would that be sufficient for advanced users?

tux-rampage · 2018-10-19T18:39:29Z

@weierophinney I walked through the topics. I thought I had adressed your remarks previously, but I seem to have missed to push them from my notebook. Anyways I made sure this time ;-)

As for your remarks on the templates, these are test covered to ensure specific results. I double checked these expectaions in test/_files/expected-codegen-results/ to address the issues from your review.

I also opened Issue #39 to adress the upcomming coding standard v2. The current one may haunt us with any PR since the tooling does not even complain - which is as frustrating for contributors as for reviewers.

Ocramius · 2018-10-21T21:59:31Z

Even though implementing a custom resolver is a rare use case I'd like to give users the freedom with an explicit contract when they know what they're doing.

You said it's internal: if you aren't ready for it to be public API in a release, then mark it as @internal. The marker can be removed later on, once the API is stable enough.

If you don't know exactly how to document it, then it is probably not ready for the wider audience either. Give it your best shot for now. Maybe even rename the methods, if you think that would aid clarity, but don't make it a public interface if it's not ready.

For now this is an option until it's stable enough to become part of the public api.

tux-rampage · 2018-10-22T12:19:31Z

You said it's internal: if you aren't ready for it to be public API in a release, then mark it as @internal. The marker can be removed later on, once the API is stable enough.

Yes, that sounds like the best way to go, thanks. When I find the time I'll review the resolver parts and check if this has a stable api via interfaces, so that we can cover it an advanced documentation section - when this is the case @internal can be dropped.

If you don't know exactly how to document it, then it is probably not ready for the wider audience either. Give it your best shot for now.

I already added a lot of PHPDoc explaining the purpose of this interface to the maintainers. It now also has the @internal annotation. Could you please have a look if this is sufficient?

Thaks again for your support on this, @Ocramius 👍

weierophinney

Honestly, this looks great! There's some CS consistency, but you have another PR open for that.

As I've noted below, the only thing I think needs changes is the CHANGELOG: you need to elaborate a bit on which contract is being defined, and why, as that's the user-facing aspect.

weierophinney · 2018-10-22T14:19:24Z

CHANGELOG.md

@@ -14,6 +14,15 @@ All notable changes to this project will be documented in this file, in reverse
  factory `Zend\Di\Container\GeneratorFactory` for creating a
  `Zend\Di\CodeGenerator\InjectorGenerator` instance with zend-servicemanager.

+- [#38](https://github.com/zendframework/zend-di/pull/38) adds `Zend\Di\Resolver\InjectionInterface`
+  to define the contract 


Define which contract? Elaborate here, as this is where users get the information they need to determine the impact of the changes.

Right, that's related to the discussion above. This will need a change indeed.

Besides this little bit, this is ready to 🚢

Ocramius · 2018-10-22T16:53:13Z

CHANGELOG.md

@@ -14,6 +14,15 @@ All notable changes to this project will be documented in this file, in reverse
  factory `Zend\Di\Container\GeneratorFactory` for creating a
  `Zend\Di\CodeGenerator\InjectorGenerator` instance with zend-servicemanager.

+- [#38](https://github.com/zendframework/zend-di/pull/38) adds `Zend\Di\Resolver\InjectionInterface`
+  to define the contract 


Besides this little bit, this is ready to 🚢

Ocramius

🚢

All feedback handled

tux-rampage · 2018-10-23T09:30:58Z

🙌 Thank you all for your support on this. I'm prepping the merge as soon as Travis and coveralls finished.

tux-rampage · 2018-10-23T09:32:40Z

Thank's @Ocramius - that was quick 😃

Ocramius · 2018-10-23T09:36:16Z

Thank YOU for the patience in following this. We should likely decide whether to close off master and do a release, btw.

tux-rampage · 2018-10-23T09:44:57Z

Indeed I was also going to suggest this. I can take care of it to learn how to do it. To do so I would first do the release steps as described in the Maintainers guideline and push it to my repo so one of you can have a look if I did it correctly.

Ocramius · 2018-10-23T09:46:22Z

I can verify that later today - remember to sign any tags you create.

tux-rampage · 2018-10-23T14:04:26Z

@Ocramius ready: https://github.com/tux-rampage/zend-di - Was this correct?

Ocramius · 2018-10-23T14:37:39Z

Hmm, the tag can't be recognised. Did you try uploading your public GPG key?

tux-rampage · 2018-10-23T15:00:44Z

Yes I did. That's strange. To me github shows that it's verified. I had to re-push the tag since I first signed with the wrong key, but that was before my comment here. hold on i'm checking this on my mobile ...

tux-rampage · 2018-10-23T15:05:24Z

My mobile phone shows verified as well: https://github.com/tux-rampage/zend-di/tags
Could github simply need some time to sync this through all of their servers?

tux-rampage added 6 commits August 29, 2018 10:40

Define tests to reduce injection complexity

28062d3

An interface allows more flexibility to implementers/consumers. The setter/getter for parameter name is polluting the instance's state and this information is not really needed. It's the job of the resolver to provide the name/injection link.

Add missing test for abstract injector

d6d6db9

Update test cases

afc8a1d

Reduce code complexity

6e44615

Refactor injection implementation

0a59dee

Fix coding style

6ab8df3

Ocramius reviewed Aug 29, 2018

View reviewed changes

Ocramius suggested changes Aug 29, 2018

View reviewed changes

tux-rampage added 5 commits August 29, 2018 15:05

Add tests for factory generator output

1ff188f

This may be time to get rid of the zend-code dependency

Add special case for exporting null

f0691e0

PHP will export NULL, which conflicts with the ZF coding standard.

Change deprecation of AbstractInjection to file level

d49877c

Simplify factory from string test

9b9cc4f

Remove redundant return type in php doc

49eb25c

tux-rampage commented Aug 29, 2018

View reviewed changes

test/CodeGenerator/AbstractInjectorTest.php Show resolved Hide resolved

tux-rampage added the WIP label Aug 31, 2018

tux-rampage added 7 commits August 31, 2018 13:02

Add missing deprecation notice

6547e0a

Fix coding style

9d3c644

Fix build failure due to deprecation notice at file level

19ed2b2

Fix output expectations

a0416bd

Simplify factory generator by removing zend code

90321e2

Make generator more explicit

40d581f

Adding a strict return type and throw exceptions on failure makes the implementation predictable.

Update changelog

138bd99

tux-rampage commented Aug 31, 2018

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

tux-rampage added 3 commits September 3, 2018 11:56

Re-structure generator tests

8a859d8

Utilize SplFileObject for output

0a46b8c

This will not trigger a warning as file_put_contents would. Instead it will throw when opening the file for read access fails.

Refactor autoload generator to remove zend-code dependencies

edcf6ed

froschdesign reviewed Sep 3, 2018

View reviewed changes

tux-rampage added 2 commits September 4, 2018 10:36

Update composer dependencies

bfcf869

Remove zend-code as dependency

edd5551

Add doc comments to InjectionInterface

6135cd9

Mark InjectionInterface internal

f2fbf38

For now this is an option until it's stable enough to become part of the public api.

weierophinney reviewed Oct 22, 2018

View reviewed changes

Ocramius suggested changes Oct 22, 2018

View reviewed changes

tux-rampage added 2 commits October 23, 2018 11:25

Update changelog entry for InjectionInterface

64cf9da

Rephrase changelog entry for InjectionInterface

bcbe78a

Ocramius approved these changes Oct 23, 2018

View reviewed changes

Ocramius merged commit ea06ffc into zendframework:develop Oct 23, 2018

Ocramius assigned Ocramius and unassigned tux-rampage Oct 23, 2018

tux-rampage deleted the code-quality branch October 23, 2018 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code quality #38

Improve code quality #38

tux-rampage commented Aug 29, 2018 •

edited

Loading

Ocramius Aug 29, 2018

tux-rampage Aug 29, 2018

Ocramius Aug 29, 2018

Ocramius Aug 29, 2018

tux-rampage Aug 29, 2018

tux-rampage Aug 29, 2018

froschdesign Sep 3, 2018

tux-rampage Sep 3, 2018

tux-rampage Sep 4, 2018 •

edited

Loading

tux-rampage commented Oct 19, 2018

Ocramius commented Oct 19, 2018

tux-rampage commented Oct 19, 2018

tux-rampage commented Oct 19, 2018

Ocramius commented Oct 21, 2018

tux-rampage commented Oct 22, 2018

weierophinney left a comment

weierophinney Oct 22, 2018

tux-rampage Oct 22, 2018

Ocramius Oct 22, 2018

Ocramius Oct 22, 2018

Ocramius left a comment

tux-rampage commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

Ocramius commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

Ocramius commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

Ocramius commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

Improve code quality #38

Improve code quality #38

Conversation

tux-rampage commented Aug 29, 2018 • edited Loading

TODOs:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tux-rampage Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

tux-rampage commented Oct 19, 2018

Ocramius commented Oct 19, 2018

tux-rampage commented Oct 19, 2018

tux-rampage commented Oct 19, 2018

Ocramius commented Oct 21, 2018

tux-rampage commented Oct 22, 2018

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius left a comment

Choose a reason for hiding this comment

tux-rampage commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

Ocramius commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

Ocramius commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

Ocramius commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

tux-rampage commented Oct 23, 2018

tux-rampage commented Aug 29, 2018 •

edited

Loading

tux-rampage Sep 4, 2018 •

edited

Loading