Added MakeSerializerEncoder command. #30

piotrgradzinski · 2017-11-19T09:30:14Z

@javiereguiluz - I know that this is an early stage of this repository and you've mentioned in #27 that the focus should be more on fixing current issues and problems. I would love to help on those :)

Nevertheless I hope you'll find this PR useful as a implementation of one of proposed commands from #17.

javiereguiluz · 2017-11-19T09:43:56Z

src/Command/MakeSerializerEncoderCommand.php

+    protected function writeNextStepsMessage(array $params, ConsoleStyle $io)
+    {
+        $io->text([
+            'Next: Open your new encoder class and start customizing it.',


This is not a question about your Pull Request but a general question: are these "next steps" instructions really useful?

This doesn't provide much: "Next: Open your new encoder class and start customizing it."

And this is more suited for a "demo app" or "newcomer learning resource": "Find the documentation at ..."

I propose to remove all these obvious "next steps" and only keep the ones really needed.

I think you may be right. But I still want a clear “success” message at the bottom. Maybe there’s a default, generic success message and we only override it when there really are additional steps

The exact next information is not really helpful in this case (just followed the convention from other commands).

Just to point that when you generate new command over SensioGeneratorBundle after confirmation you'll receive

You are going to generate a foo-bar command inside AppBundle bundle. Do you confirm generation [yes]? created ./src/AppBundle/Command/ created ./src/AppBundle/Command/FooBarCommand.php Generated the foo-bar command in AppBundle Everything is OK! Now get to work :).

Not sure if and how close to this maker should be.

If agreed I can refactor existing commands for that.

OK ... let's discuss about this in a separate PR and, if needed, let's make the change at once for all commands.

weaverryan

I’m happy to have this command! @dunglas does the generated class look good to you or would you suggest any changes? Is it the best practice to create just one class that’s both an encoder and a decoder?

weaverryan · 2017-11-19T09:56:03Z

composer.json

@@ -17,7 +17,8 @@
        "symfony/dependency-injection": "^3.4|^4.0",
        "symfony/filesystem": "^3.4|^4.0",
        "symfony/framework-bundle": "^3.4|^4.0",
-        "symfony/http-kernel": "^3.4|^4.0"
+        "symfony/http-kernel": "^3.4|^4.0",
+        "symfony/serializer": "^3.4|^4.0"


This should be in require-dev - it’s only needed to test the app.

ogizanagi · 2017-11-19T10:06:16Z

src/Resources/skeleton/serializer/Encoder.php.txt

+
+    public function supportsEncoding($format)
+    {
+        return '{{ format }}' === $format;


The format could be in a FORMAT class const, as done in built-in encoders:

https://github.com/symfony/symfony/blob/8eb97f5c483b57d8cec2fe999d1437fadfeae575/src/Symfony/Component/Serializer/Encoder/JsonEncoder.php#L21

@ogizanagi - makes sense. Fixed.

piotrgradzinski · 2017-11-19T10:14:18Z

@weaverryan - moved symfony/serializer to required-dev in composer.json
@ogizanagi - changed format to const.

javiereguiluz · 2017-11-19T11:38:01Z

src/Command/MakeSerializerEncoderCommand.php

+    protected function configure()
+    {
+        $this
+            ->setDescription('Creates a new custom encoder class')


What about:

Creates a new serializer encoder class

Yup, fixed in command description, success message (as is :) ) and command help.

javiereguiluz · 2017-11-19T11:41:23Z

src/Resources/skeleton/serializer/Encoder.php.txt

+        return '';
+    }
+
+    public function supportsEncoding($format)


Just asking: we can't do it right and add type hints (e.g. supportsEncoding(string $format): bool) because of the interfaces, right?

@javiereguiluz indeed. Since there is no type-hinting in the interface, trying to add some in implemented method will end up with

Fatal error: Declaration of YamlEncoder::supportsEncoding(string $format) must be compatible with EncoderInterface::supportsEncoding($format)

In fact you could at least add the return type: https://3v4l.org/GiabR.

@ogizanagi right! Done!

…methods.

yceruto · 2017-11-19T13:35:38Z

composer.json

@@ -21,7 +21,8 @@
    },
    "require-dev": {
        "symfony/phpunit-bridge": "^3.4|^4.0",
-        "symfony/process": "^3.4|^4.0"
+        "symfony/process": "^3.4|^4.0",
+        "symfony/serializer": "^3.4|^4.0"


It is really necessary? I mean, we don't do this for the current ones.

@yceruto - it was from the beginning of the repo. Correct me if I'm wrong but I think removing this should go to a separate issue.

symfony/serializer deps has been introduced in this PR, no? I think is not necessary for tests either, as tests don't check dependencies currently:
https://github.com/yceruto/maker-bundle/blob/c402dac16d4744bf0c1d159ec67bdaf0d3fbd619/tests/Command/FunctionalTest.php#L49

You are right. Removed from composer.json.

dunglas · 2017-11-19T16:02:47Z

src/Resources/skeleton/serializer/Encoder.php.txt

+        return '';
+    }
+
+    public function supportsEncoding($format): bool


?string $format should work (not tested it), same for decode.

@dunglas - unfortunately the problem remains, tested on PHP 7.1.6

Fatal error: Declaration of YamlEncoder::supportsEncoding(?string $format) must be compatible with EncoderInterface::supportsEncoding($format)

adding it relies on PHP 7.2 new features.

dunglas · 2017-11-19T16:03:31Z

src/Resources/skeleton/serializer/Encoder.php.txt

+{
+    const FORMAT = '{{ format }}';
+
+    public function encode($data, $format, array $context = array())


array $context = [] as we're on 7.1+?

Fixed. Thank you!

dunglas · 2017-11-19T16:04:05Z

src/Resources/skeleton/serializer/Encoder.php.txt

+
+class {{ encoder_class_name }} implements EncoderInterface, DecoderInterface
+{
+    const FORMAT = '{{ format }}';


public const?

Fixed. Thank you!

yceruto · 2017-11-20T13:12:24Z

src/Command/MakeSerializerEncoderCommand.php

+    {
+        $dependencies->addClassDependency(
+          Serializer::class,
+          'serializer'


use 4 space for each level of indentation.

Fixed. Thank you!

piotrgradzinski · 2017-11-21T10:29:19Z

@weaverryan - are you happy with the recent changes?

weaverryan

I'm super happy :). Thanks!

piotrgradzinski · 2017-11-22T10:14:10Z

Guys, I know that in the light of @javiereguiluz recent post about this bundle the approach is to discuss the potential new command over a ticket but I am wondering whether this PR can be merged? It's been discussed in #17 and I hope make:serializer:encode code is good enough to be merged.

Thank you for your attention!

weaverryan · 2017-11-28T14:52:27Z

Thanks Piotr for working on this feature, this is much appreciated.

@javiereguiluz

This PR was squashed before being merged into the 1.0-dev branch (closes #30). Discussion ---------- Added MakeSerializerEncoder command. @javiereguiluz - I know that this is an early stage of this repository and you've mentioned in #27 that the focus should be more on fixing current issues and problems. I would love to help on those :) Nevertheless I hope you'll find this PR useful as a implementation of one of proposed commands from #17. Commits ------- 6079823 MakeSerializerEncoder - indentation fix. 6dc2d2f MakeEncoderSerializer - template fixes. 74e098b MakeEncoderSerializer - removing serializer dependency. 7051ecb MakeEncoderSerializer - adding typehint for return value on supports methods. 5cfc4f8 MakeEncoderSerializer - changed command description. f02ce64 MakeEncoderSerializer move format to constant. df17425 Moving symfony/serializer lib to require-dev e3c3d09 Added MakeSerializerEncoder command.

weaverryan · 2017-11-28T14:52:46Z

And thanks also for making the quick changes! Woo!

stof · 2017-11-28T15:01:24Z

I think making custom normalizer is much more likely to happen than custom encoders though.

piotrgradzinski · 2017-11-28T15:28:26Z

@stof - this is a good idea I think. If you are OK with that I could start new issue to discuss this further. If there be a will for that I would love to prepare such command.

Added MakeSerializerEncoder command.

e3c3d09

javiereguiluz reviewed Nov 19, 2017

View reviewed changes

weaverryan requested changes Nov 19, 2017

View reviewed changes

Moving symfony/serializer lib to require-dev

df17425

ogizanagi reviewed Nov 19, 2017

View reviewed changes

MakeEncoderSerializer move format to constant.

f02ce64

ogizanagi approved these changes Nov 19, 2017

View reviewed changes

javiereguiluz reviewed Nov 19, 2017

View reviewed changes

piotrgradzinski added 2 commits November 19, 2017 12:45

MakeEncoderSerializer - changed command description.

5cfc4f8

MakeEncoderSerializer - adding typehint for return value on supports …

7051ecb

…methods.

yceruto reviewed Nov 19, 2017

View reviewed changes

dunglas reviewed Nov 19, 2017

View reviewed changes

piotrgradzinski added 2 commits November 20, 2017 09:41

MakeEncoderSerializer - removing serializer dependency.

74e098b

MakeEncoderSerializer - template fixes.

6dc2d2f

yceruto reviewed Nov 20, 2017

View reviewed changes

MakeSerializerEncoder - indentation fix.

6079823

weaverryan approved these changes Nov 21, 2017

View reviewed changes

dunglas approved these changes Nov 21, 2017

View reviewed changes

weaverryan merged commit 6079823 into symfony:master Nov 28, 2017

lyrixx mentioned this pull request Oct 10, 2018

Added a command to create a Normalizer #287

Closed

Added MakeSerializerEncoder command. #30

Added MakeSerializerEncoder command. #30

Conversation

piotrgradzinski commented Nov 19, 2017 • edited Loading

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

weaverryan 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

piotrgradzinski commented Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrgradzinski Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrgradzinski Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

dunglas Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Nov 19, 2017 • edited Loading

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

piotrgradzinski commented Nov 21, 2017

weaverryan left a comment

Choose a reason for hiding this comment

piotrgradzinski commented Nov 22, 2017

weaverryan commented Nov 28, 2017

weaverryan commented Nov 28, 2017

stof commented Nov 28, 2017

piotrgradzinski commented Nov 28, 2017

piotrgradzinski commented Nov 19, 2017 •

edited

Loading

piotrgradzinski commented Nov 19, 2017 •

edited

Loading

piotrgradzinski Nov 19, 2017 •

edited

Loading

yceruto Nov 19, 2017 •

edited

Loading

piotrgradzinski Nov 20, 2017 •

edited

Loading

dunglas Nov 19, 2017 •

edited

Loading

dunglas Nov 19, 2017 •

edited

Loading