Add missing deprecations #222

greg0ire · 2016-12-03T14:00:33Z

I am targetting this branch, because this is BC (addind a deprecation notice).

Changelog

### Deprecated
- not specifying the `default_formatter` configuration node is deprecated
- specifying a logger through the `SonataFormatterBundle\Formatter\Pool` constructor is deprecated in favor of specifying it through `setLogger` method
- not specifying a default formatter to the `SonataFormatterBundle\Formatter\Pool` constructor

Subject

See #221

It is more concise.

greg0ire · 2016-12-03T14:03:56Z

Please review @sagikazarmark

sagikazarmark · 2016-12-03T18:26:39Z

DependencyInjection/SonataFormatterExtension.php

        if (!isset($config['default_formatter'])) {
+            @trigger_error(<<<'EOT'
+Not setting the default_formatter configuration node is deprecated since 3.x,


Well, I don't really like this kind of line splitting within strings. Does not make things easier to read IMO.

Also, maybe add error triggering in the pool as well?

Well, I don't really like this kind of line splitting within strings. Does not make things easier to read IMO.

I could argue that it is a matter of taste, but is it? I mean, you did not have to scroll horizontally to read this piece of text, so it really was easier to read in an objective way. If I didn't, the line would be ~ 130 chars wide because of indention. I don't want to end up with code like this, that must be browsed in 2 dimensions.

Also, maybe add error triggering in the pool as well?

I thought about it, but how would you achieve that? Because you can't trigger a deprecation if you don't offer a new way. Would you drop the type hinting and do duck typing? It's possible but a bit ugly. Another possiblity I thought about: make the logger mandatory too (people that do not want to log can use a null logger). If we do that, we are no longer forced to switch arguments. What do you think?

I'm gonna go ahead and do it while I have some spare time.

@sagikazarmark : I pushed a new commit ( 2bab6fd ) with null logger deprecated, please tell me what you think.

ping @sagikazarmark ?

No response. @sonata-project/contributors please review.

Using Nowdoc for this short text is to excessive.

Same for the other code fragment.

What would you do ? Concatenation? All on one line? I'll try concatenation if everyone is put off by this.

@sagikazarmark :

Well, I don't really like this kind of line splitting within strings. Does not make things easier to read IMO.

By that, did you mean the string the code, or the produced string? I think I might have misunderstood this.

greg0ire · 2016-12-09T16:09:41Z

@sagikazarmark @core23 here is version without splitting the produced string. I don't like it because I have to be careful to add the space on line 63 if I want to produce a nice string, and because it's cumbersome to create and edit (if I have to add something in the middle of the string, I have to move the point where I use concatenation). Also, notice how this would go beyond the 120 chars soft limit and make people scroll to review if I didn't use concatenation. So no, I don't think nowdoc is excessive here, but I listen when both of you say you do not like it. Complying because I want to keep the ball rolling.

sagikazarmark · 2016-12-09T23:11:29Z

For me enforcing the char limit in every case doesn't sound like a good idea. Keep in mind that those messages are for logs or error pages, not for the code itself. Also, splitting arguments into multiple lines does not add ANY overhead, the concatenation does, even if it is negligible, but it's there for the sake of char limiting? I am definitely against it.

sagikazarmark · 2016-12-09T23:13:52Z

Formatter/Pool.php

@@ -39,10 +39,19 @@ class Pool
     */
    public function __construct(LoggerInterface $logger = null, $defaultFormatter = null)
    {
+        // NEXT_MAJOR: make both parameters required


Do we really need a logger here? Personally I would prefer leaving it optional and using the NullLogger as a fallback so that there is no need to check the existence of the logger. I would even consider removing it from the constructor and using the LoggerAwareInterface.

I thought of it, but it makes BC harder to achieve. And anyway, this class is mostly used via the DIC imo, which makes providing the logger a non-issue. But also makes BC less an issue. Will think about it more.

So I thought about it, and I think BC might be actually achievable with some func_num_args magic. Let me give it a try.

greg0ire · 2016-12-10T01:42:44Z

Keep in mind that those messages are for logs or error pages, not for the code itself.

That's their main purpose indeed, but the message should also be reviewed, and might be edited, or just read by someone who wished to understand what the condition he just read is about.

Also, splitting arguments into multiple lines does not add ANY overhead, the concatenation does, even if it is negligible, but it's there for the sake of char limiting? I am definitely against it.

I have zero concern about overhead here, that would be a typical case of premature performance optimization. What I want to optimize for here is readability and maintainability. I want to avoid things you can see in Symfony like this kind message I linked to earlier where you have to scroll again and again, and things you can see in Doctrine, like this kind of unmaintainable message. Heredoc was IMO made precisely for this kind of case, and if you start not using it with short cases, you can very well get carried away with long strings like in Symfony or Doctrine. That's what I want to avoid here.

I can see though how heredoc / nowdoc have their own problems :

it breaks the indention;
the delimiters are long to write, and look ugly.

I'd like to promote them however, because the advantages show up quickly, once you go past parsing the delimiters, the enclosed message is very readable, and in a very objective and justifiable manner :

no concatenation in the middle of the string, which makes it easy to edit, and, arguably, read;
no need to scroll, which makes it indubitably easier to review;
you can use both single and double quotes without any escaping, which makes it easier to write.

Your main point might be "one log should be equal to one line". I tend to instinctively agree with that, but can't say why. I mean, what are you going to do with that, count the number of logs? Or is it about parsing logs with ease? Anyway, it this rule is so important, shouldn't it be the concern of the logging library, which could easily strip carriage returns and extra whitespace?

sagikazarmark · 2016-12-10T16:00:12Z

I think all arguments are given, it's up to the maintainers to decide.

greg0ire · 2016-12-10T16:08:56Z

I think all arguments are given, it's up to the maintainers to decide.

Indeed. I'm working on the LoggerAwareInterface implementation right now, almost finished. Not using heredoc because I don't want this discussion to hold this PR back.

greg0ire · 2016-12-10T16:20:15Z

Formatter/Pool.php

+                ' This argument will become mandatory in 4.0.',
+                __METHOD__
+            ), E_USER_DEPRECATED);
+        }


@sagikazarmark : I'm done with the LoggerAwareInterface implementation (I think). Looks good?

sagikazarmark · 2016-12-10T16:26:03Z

Formatter/Pool.php

@@ -34,15 +36,43 @@ class Pool
    protected $logger;


If you use the trait from the psr/log package you can drop this

Scheduled for next major

sagikazarmark · 2016-12-10T16:26:14Z

Formatter/Pool.php

+        }
+    }
+
+    final public function setLogger(LoggerInterface $logger)


This as well

will do 👍

Oh wait, we don't have traits yet (php 5.3). Let's add a NEXT_MAJOR tag instead.

sagikazarmark · 2016-12-10T16:27:44Z

Formatter/Pool.php

+            return $this->logger;
+        }
+
+        return new NullLogger();


Hm, not sure about lazy initialization. I tend to avoid it. Also, you didn't spare anything, the same condition is still there. I would probably set a null logger in the constructor by default.

Nice suggestion, I'll do that.

greg0ire · 2016-12-10T16:35:55Z

@sagikazarmark : all good now?

sagikazarmark · 2016-12-10T19:10:47Z

Formatter/Pool.php

-
-        // TODO: This should become a required first parameter when the major version is changed
+        $this->logger = new NullLogger();
+        // NEXT_MAJOR: remove this block


Add a new line here?

sagikazarmark · 2016-12-10T19:12:14Z

Formatter/Pool.php

+                __METHOD__
+            ), E_USER_DEPRECATED);
+
+            return;


Returning here is not a good idea as the default formatter is null and we should trigger a deprecation error. Just set the defaultFormatter to null and let the rest of the logic run.

sagikazarmark · 2016-12-10T19:12:31Z

Formatter/Pool.php

+            ), E_USER_DEPRECATED);
+
+            return;
+        }


Add a newline here?

sagikazarmark · 2016-12-10T19:14:26Z

Formatter/Pool.php

+
+            return;
+        }
+        if (func_num_args() == 1 && $defaultFormatter instanceof LoggerInterface) {


Elseif? No need to evaluate this condition if there are two arguments.

sagikazarmark · 2016-12-10T19:15:22Z

Formatter/Pool.php

+        }
+        if (func_num_args() == 1 && $defaultFormatter instanceof LoggerInterface) {
+            $this->logger = $defaultFormatter;
+            @trigger_error(sprintf(


Shouldn't this error be triggered if there are two arguments too?

Maybe: if this->logger is set then trigger error else this->logger = new NullLogger().

sagikazarmark · 2016-12-10T19:18:54Z

Formatter/Pool.php

+     *
+     * @param LoggerInterface will be used to report errors
+     */
+    final public function setLogger(LoggerInterface $logger)


Not sure how this works in the interface/trait, but I would use the exact same signature to avoid breaking BC (though removing final is not a BC breaking operation). As far as I remember the trait exposes a fluent interface. I might be wrong, but if not, this should be fluent as well.

The interface is not fluent, and as you said, removing final is not a BC-break, but there is little point to making it final if we are sure it won't be in the future.

greg0ire · 2016-12-11T00:13:08Z

@sagikazarmark : thanks for your review, I reworked the constructor entirely with your remarks in mind, hopefully the logic is better this time.

sagikazarmark · 2016-12-11T07:18:45Z

Formatter/Pool.php

+            $this->logger = new NullLogger();
+        }
+        // NEXT_MAJOR: make defaultFormatter required
+        if (is_null($defaultFormatter)) {


This issue is still not resolved: if the first and only arg is a logger, default formatter won't be null here, missing the deprecation trigger. Either check the class property, or set the default formatter on line 51 to null.

Oh right, I missed that, sorry! I'll do the former.

greg0ire · 2016-12-11T10:58:11Z

@sagikazarmark : fixed the default formatter check

sagikazarmark · 2016-12-11T11:35:12Z

Formatter/Pool.php

    {
-        $this->logger = $logger;
+        if (func_num_args() == 2) {


NEXT_MAJOR: Remove this block

I moved the comment to line 34, to explain that line 35 must be kept, which would be harder to convey from line 45.

yoda please

sagikazarmark · 2016-12-11T11:36:12Z

Formatter/Pool.php

+        // NEXT_MAJOR: keep the else block only
+        if ($this->logger) {
+            @trigger_error(sprintf(
+                'Providing a logger to %s through the constructor is deprecated since 3.x'.


This will become "Providing a logger to Pool::__construct through the constructor...". Is this intended?

This is where the string split kicks in as well: if you change the message, you need to rearrange the strings as well.

Oh right, changing it to __CLASS__

This is where the string split kicks in as well: if you change the message, you need to rearrange the strings as well.

Yeah but if you used nowdoc it's a breeze: you join the lines together again, hit gqq and you're done. If you use concatenation… that's another story.

sagikazarmark · 2016-12-11T11:39:10Z

UPGRADE-3.x.md

+
+## Deprecated unspecified Sonata\FormatterBundle\Formatter\Pool defaultFormatter
+
+Instanciating the `Sonata\FormatterBundle\Formatter\Pool` class should be done


Instantiating

One of my coworkers just made this mistake this week! Now I'm contaminated!

sagikazarmark · 2016-12-11T11:40:09Z

UPGRADE-3.x.md

+## Deprecated specifying a logger in the Sonata\FormatterBundle\Formatter\Pool constructor
+
+Providing a logger to a `Sonata\FormatterBundle\Formatter\Pool` instance is
+optional, and optional dependencies should be provided through a setter.


IMO this is arguable, I would just stick to facts: Logger can now be injected via setter instead of passing it to the constructor.

That's right, let's not make the upgrade file up to debate

sagikazarmark · 2016-12-11T11:40:37Z

UPGRADE-3.x.md

+    default_formatter: my_formatter
+```
+
+## Deprecated specifying a logger in the Sonata\FormatterBundle\Formatter\Pool constructor


Maybe say injecting?

That's better, yes 👍

sagikazarmark · 2016-12-11T11:40:50Z

UPGRADE-3.x.md

+
+Before:
+
+```


Missing php?

sagikazarmark · 2016-12-11T11:41:53Z

UPGRADE-3.x.md

+$pool->setLogger($myLogger);
+```
+
+## Deprecated unspecified Sonata\FormatterBundle\Formatter\Pool defaultFormatter


Deprecated unspecified default formatter in Sonata\FormatterBundle\Formatter\Pool constructor?

sagikazarmark · 2016-12-11T11:42:28Z

UPGRADE-3.x.md

+## Deprecated unspecified Sonata\FormatterBundle\Formatter\Pool defaultFormatter
+
+Instanciating the `Sonata\FormatterBundle\Formatter\Pool` class should be done
+with both a `$defaultFormatter` argument.


Good catch!

sagikazarmark · 2016-12-11T11:43:07Z

Added a few more comments, but the code looks ok to me.

greg0ire · 2016-12-11T11:57:10Z

Added a few more comments, but the code looks ok to me.

Hurray! @sonata-project/contributors , can you please give this a final review?

sagikazarmark

Apart from how exception messages are handled I approve. It should be up to the maintainers to decide if messages should be split, I am definitely against it.

OskarStark · 2016-12-12T08:44:18Z

Formatter/Pool.php

 use Twig_Environment;
 use Twig_Error_Syntax;
 use Twig_Sandbox_SecurityError;

-class Pool
+class Pool implements LoggerAwareInterface


this is per definition a BC break IMO

I don't think so. Please tell me how it would be one.

I am curious about the reasoning as well. 😄

thats why i said per definition, but if we fullfill the interface, everything is fine.

Just for the theoretical debate: how is it a BC break per definition? If you don't actually implement the interface your code will be broken.

OskarStark · 2016-12-12T08:44:32Z

Formatter/Pool.php

+            list($logger, $defaultFormatter) = func_get_args();
+            $this->logger = $logger;
+            $this->defaultFormatter = $defaultFormatter;
+        } elseif (func_num_args() == 1) {


yoda please

You can't assign a value to a function call. So this would protect us from nothing, and make things less readable.

Just wanted to say that. Yoda for function call?????

@sagikazarmark : I always said the Jedi council has too much influence :) Look how carried away they can get

Well, for me THIS is the point where sanity should be superior to consistency.

Ok, how you like, I really like the reading more

Yeah but then again you guys put verbs at the end of a sentence 🇩🇪 ;)

OskarStark · 2016-12-12T08:44:40Z

Formatter/Pool.php

    {
-        $this->logger = $logger;
+        if (func_num_args() == 2) {


yoda please

OskarStark · 2016-12-12T08:45:05Z

Formatter/Pool.php

+        if ($this->logger) {
+            @trigger_error(sprintf(
+                'Providing a logger to %s through the constructor is deprecated since 3.x'.
+                ' and we no longer be possible in 4.0'.


we or will?

@OskarStark fixed! Good catch!

This will allows us to avoid having to switch arguments when making the default formatter mandatory, thus getting a good upgrade path through the deprecation system. If no logger is provided, a NullLogger is used.

This can be used by tools like Sentry to display a nice stack trace.

greg0ire · 2016-12-13T10:12:49Z

Thanks @OskarStark , @core23, final review and merge? Or maybe we can merge like that since @sagikazarmark has approved?

sagikazarmark · 2016-12-13T11:56:53Z

I still have faith that @core23 will veto the string splitting.

greg0ire · 2016-12-13T12:24:20Z

I still have faith that @core23 will veto the string splitting.

Noooooo! Please don't do that! I already abandoned nowdoc because you asked, please don't coerce me into commiting any other atrocity. Plus, the produced string isn't split, isn't this what you wanted?

@sagikazarmark joking aside, I'd like to know :

is this really ok for you?
If yes, do you really think @OskarStark would have caught this hadn't it be split?
if not :
1. if not what is an acceptable char limit for you?
2. would you always do this or is there also a limit where you would switch to nowdoc?

sagikazarmark · 2016-12-13T12:38:42Z

Whoa, too much questions and logical links between them.

I think I already expressed that exception messages are NOT primarily for developers to read (or at least not as part of the code, but in logs or on error pages), while code needs to be read, so char limit makes sense there.

I also said that any kind of string magic adds an overhead, even if negligible. Anything like that JUST FOR readability is totally unacceptable IMO.

Most IDEs break the long lines automatically anyway, so the readability issue is not an issue then.

Char limit is not for string literals, it's for code. How do you apply char limit to a JSON document for example? The only reason you can do it here is that you can do it programatically. GOTO two points above

In the linked doctrine case I would probably use sprintf, since they mostly use concatenation because of the parameters in the string.

This is also a thing where sanity should be over consistency: you cannot apply a char limit anywhere, or you can, but it comes with a price which I don't want to pay.

I think enough reasons and arguments said, while on the other side I only see blindly applying char limit (no offense) and bringing the readability argument which I IMO is not a valid one (see reasoning above).

greg0ire · 2016-12-13T12:53:14Z

Whoa, too much questions and logical links between them.

I numbered them!

NOT primarily for developers

Primarily. Still, it can be helpful for them. In our case, I think it clarifies a lot of things. So yes, char limit makes sense there too IMO, and makes sense in any file that is considered code (as opposed to data). But maybe that's where the argument boils down to? You view exception messages as pure data, while I think of them more as metadata associated with a piece of code.

How do you apply char limit to a JSON document for example

I don't store JSON documents in code directly… that's data, not code.

I also said that any kind of string magic adds an overhead, even if negligible.

Nowdoc doesn't add any, does it?

I think enough reasons and arguments said, while on the other side I only see blindly applying char limit (no offense) and bringing the readability argument which I IMO is not a valid one (see reasoning above).

No offense taken, I'm happy with this dicussion, and hope you are too. I know it's a very pedantic debate, but it affects a lot of code.

sagikazarmark · 2016-12-13T13:01:03Z

Well, that's why I said it's a matter of priorities and personal taste and should be up to the maintainers to decide. I don't actually think that any of our reasoning can be superior to each other. One thing I would keep in mind: consistency with the rest of the code base is important too, so if you want to raise this issue, I would be happier with a general one for all the projects.

greg0ire · 2016-12-13T13:02:57Z

consistency with the rest of the code base is important too, so if you want to raise this issue, I would be happier with a general one for all the projects.

I'll make sure to do that and ping you when I do. In the meantime, I think we can merge this as is b/c we already use concatenation and heredoc in Sonata for that purpose (guess who did this). The contributing guide even shows concatenation.

sagikazarmark · 2016-12-13T13:21:57Z

(guess who did this)

Bah, abominations slip in unnoticed is a downside of open source projects.

core23 · 2016-12-13T18:37:28Z

DependencyInjection/SonataFormatterExtension.php

        if (!isset($config['default_formatter'])) {
+            @trigger_error(
+                'Not setting the default_formatter configuration node is deprecated since 3.x,'.


I wouldn't split this string in two parts just for the character limit, because searching for this particular string in the whole projects wouldn't work later.

This is a general discussion I don't want to start

So diplomatic.... 😝

This is a general discussion I don't want to start

Well why are you starting it then? All jokes aside, you both aprroved, so I'll just merge. But stay tuned for a real discussion about this, I'll make ~~a blog post~~ an issue soon.

sagikazarmark · 2016-12-13T19:38:00Z

https://twitter.com/sagikazarmark/status/808757703250219008

greg0ire · 2016-12-13T21:03:45Z

See sonata-project/dev-kit#218

Add NEXT_MAJOR marker

965706f

It is more concise.

greg0ire added the minor label Dec 3, 2016

SonataCI added the review required label Dec 3, 2016

greg0ire force-pushed the add_missing_deprecations branch from b9c0c6d to 5d69321 Compare December 3, 2016 14:03

greg0ire mentioned this pull request Dec 3, 2016

Drop old versions #221

Merged

4 tasks

sagikazarmark reviewed Dec 3, 2016

View reviewed changes

greg0ire force-pushed the add_missing_deprecations branch 6 times, most recently from 6c46ae8 to c7ff31a Compare December 9, 2016 16:00

Prepare making the default_formatter mandatory

913cd1d

greg0ire force-pushed the add_missing_deprecations branch from c7ff31a to f54e1d4 Compare December 9, 2016 16:01

sagikazarmark reviewed Dec 9, 2016

View reviewed changes

greg0ire force-pushed the add_missing_deprecations branch 3 times, most recently from 35b207c to b5e1677 Compare December 10, 2016 16:15

greg0ire commented Dec 10, 2016

View reviewed changes

sagikazarmark reviewed Dec 10, 2016

View reviewed changes

greg0ire force-pushed the add_missing_deprecations branch 2 times, most recently from bd3340f to aa10fee Compare December 10, 2016 16:35

sagikazarmark reviewed Dec 10, 2016

View reviewed changes

greg0ire force-pushed the add_missing_deprecations branch from 842eb8d to 15134b1 Compare December 11, 2016 00:12

sagikazarmark reviewed Dec 11, 2016

View reviewed changes

greg0ire force-pushed the add_missing_deprecations branch from 15134b1 to 1c2394e Compare December 11, 2016 10:57

sagikazarmark reviewed Dec 11, 2016

View reviewed changes

greg0ire force-pushed the add_missing_deprecations branch from 1c2394e to 54194d0 Compare December 11, 2016 11:56

sagikazarmark approved these changes Dec 11, 2016

View reviewed changes

OskarStark requested changes Dec 12, 2016

View reviewed changes

greg0ire added 3 commits December 12, 2016 10:27

Move optional dependency out of the constructor

19f6516

This will allows us to avoid having to switch arguments when making the default formatter mandatory, thus getting a good upgrade path through the deprecation system. If no logger is provided, a NullLogger is used.

Avoid long lines

aea3e98

Provide the special exception context key

32b654b

This can be used by tools like Sentry to display a nice stack trace.

greg0ire force-pushed the add_missing_deprecations branch from 54194d0 to 32b654b Compare December 12, 2016 09:27

OskarStark approved these changes Dec 13, 2016

View reviewed changes

core23 reviewed Dec 13, 2016

View reviewed changes

core23 approved these changes Dec 13, 2016

View reviewed changes

greg0ire merged commit e443906 into sonata-project:3.x Dec 13, 2016

greg0ire deleted the add_missing_deprecations branch December 13, 2016 19:34


		## Deprecated unspecified Sonata\FormatterBundle\Formatter\Pool defaultFormatter

		Instanciating the `Sonata\FormatterBundle\Formatter\Pool` class should be done


		Before:

		```

Add missing deprecations #222

Add missing deprecations #222

Conversation

greg0ire commented Dec 3, 2016 • edited Loading

Changelog

Subject

greg0ire commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Dec 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Dec 4, 2016 • 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

greg0ire commented Dec 9, 2016 • edited Loading

sagikazarmark commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Dec 10, 2016 • edited Loading

sagikazarmark commented Dec 10, 2016

greg0ire commented Dec 10, 2016

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

greg0ire Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Dec 10, 2016

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

greg0ire commented Dec 11, 2016

Choose a reason for hiding this comment

greg0ire Dec 11, 2016 • edited Loading

Choose a reason for hiding this comment

greg0ire commented Dec 11, 2016

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

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

sagikazarmark commented Dec 11, 2016

greg0ire commented Dec 11, 2016

sagikazarmark 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

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

greg0ire commented Dec 3, 2016 •

edited

Loading

greg0ire Dec 3, 2016 •

edited

Loading

greg0ire Dec 4, 2016 •

edited

Loading

greg0ire commented Dec 9, 2016 •

edited

Loading

greg0ire commented Dec 10, 2016 •

edited

Loading

greg0ire Dec 10, 2016 •

edited

Loading

greg0ire Dec 11, 2016 •

edited

Loading

greg0ire Dec 13, 2016 •

edited

Loading