improve factories documentation #161

dbu · 2016-10-18T08:29:48Z

dbu · 2016-10-18T08:30:23Z

message.rst

@@ -13,6 +13,3 @@ This package contains various PSR-7 tools which might be useful in an HTTP workf
 * Message decorators
 * Message factory implementations for Guzzle PSR-7, Diactoros and Slim Framework.
 * Cookie implementation
-
-.. _message-factory:


i am surprised sphinx did not complain about the duplicated reference target. it makes no sense to have this here, its in message-factory.rst

dbu · 2016-10-18T08:30:57Z

message/message-factory.rst

@@ -1,57 +1,124 @@
 .. _message-factory:


this file is still named message-factory.rst, which shows in the url. but i think as the package is called message-factory as well, its ok to keep that name.

sagikazarmark · 2016-10-18T08:32:09Z

message/message-factory.rst

+        new DiactorosUriFactory()
+    );
+
+You could also use :doc:`/discovery` to automatically find an available factory::


This part probably recommends against how we usually use discovery (inside the client with optional arguments).

Yes, I think we should update this to use optional arguments.

indeed. will change this example

sagikazarmark · 2016-10-18T08:33:41Z

message/message-factory.rst


-Message Factory
-===============
+HTTP Factories


Although I agree that they are not really Message Factories in most places we refer to them as message factories. The file name also says message-factory, so this could be a little bit confusing.

that was the initial question in #141. i find "Message Factory" wrong here. what should we call it? "Message Factories" is wrong, uri and stream are not messages. other ideas? or should i hunt through the doc and change "message factory" to "http factory" where i find that?

should i hunt through the doc and change "message factory" to "http factory" where i find that?

I am all for consistency.

i adjusted some of the naming. in many places, we do explicitly mean a message factory and not any http factory.

Nyholm · 2016-10-18T08:41:11Z

message/message-factory.rst

+    $apiClient = new ApiClient(
+        new DiactorosMessageFactory(),
+        new DiactorosStreamFactory(),
+        new DiactorosUriFactory()


You would rarely use the URI Factory since it is required by the message factory.

I suggest removing it.

hm. i want to show how to create all 3 factories in the example. and i guess there are use cases where you e.g. accept a url string and create the uri from it in your api client to do some uri manipulations before creating the request.

are you ok if we leave this as is?

I am fine with that

Nyholm · 2016-10-18T08:41:56Z

message/message-factory.rst

+        new DiactorosUriFactory()
+    );
+
+You could also use :doc:`/discovery` to automatically find an available factory::


Yes, I think we should update this to use optional arguments.

Nyholm · 2016-10-18T08:42:27Z

message/message-factory.rst

+
+.. hint::
+
+    If you only create requests, use ``RequestFactory`` in the type hint, rather


If you **only** ...

did not follow your suggestion, but tried to formulate this more explicit. better?

dbu · 2016-10-25T15:33:27Z

message/message-factory.rst

+.. hint::
+
+    If you create requests only and no responses, use ``RequestFactory`` in the
+    type hint, instead of the ``MessageFactory``. And vice versa if you create


spelling does not like this. is this proper english or not?

Actually it's latin, but should be ok. Just add it to the spellcheck list.

Already see this in english, you can also use "And conversely" if you want, but i think "vice versa" is better.

dbu · 2016-11-04T09:37:43Z

i updated this. any more feedback? anybody got an idea on the "vice versa" the spellcheck does not like?

Nyholm

Sorry for being slow reviewing this.

dbu · 2016-11-18T12:29:04Z

added versa to the spell word list, as it seems to be correct to use this in english. @sagikazarmark are you ok with this now, anything else i should add?

xabbuh · 2016-11-25T19:36:35Z

message/message-factory.rst

-The factories abstract away from this.
+While it should be possible to use every PSR-7 aware HTTP client with any
+request, URI and stream implementation, instantiating objects explicitly would
+still  tie the code to a specific implementation. If each reusable library is


A bit nitpicking: Two spaces between "still" and "tie"?

dbu · 2016-11-30T13:59:00Z

fixed double whitespace. ok like this, or any other change swe need?

sagikazarmark · 2016-12-02T11:30:36Z

Huh @dbu, you've got some nerves for doing the ugliest job dev can do: docs. 😝

dbu · 2016-12-02T17:43:31Z

i find it something extermly important :-) and sometimes i even enjoy doing it

sagikazarmark added the in progress label Oct 18, 2016

dbu commented Oct 18, 2016

View reviewed changes

sagikazarmark requested changes Oct 18, 2016

View reviewed changes

dbu force-pushed the factories-doc-improvements branch from 3d10ef3 to 600f259 Compare October 18, 2016 08:36

Nyholm reviewed Oct 18, 2016

View reviewed changes

dbu force-pushed the factories-doc-improvements branch from 600f259 to afdb9f3 Compare October 20, 2016 06:48

improve factories documentation. fix #141

3c26e59

dbu force-pushed the factories-doc-improvements branch from afdb9f3 to c6e5e49 Compare October 25, 2016 14:53

dbu commented Oct 25, 2016

View reviewed changes

Nyholm approved these changes Nov 18, 2016

View reviewed changes

dbu force-pushed the factories-doc-improvements branch from c6e5e49 to 7e49895 Compare November 18, 2016 12:28

xabbuh reviewed Nov 25, 2016

View reviewed changes

adjustion some mentions of message factory to HTTP factory

3eca342

dbu force-pushed the factories-doc-improvements branch from 7e49895 to 3eca342 Compare November 30, 2016 13:58

sagikazarmark approved these changes Dec 2, 2016

View reviewed changes

sagikazarmark merged commit 0f2b962 into master Dec 2, 2016

sagikazarmark removed the in progress label Dec 2, 2016

sagikazarmark deleted the factories-doc-improvements branch December 2, 2016 11:29


		.. hint::

		If you only create requests, use ``RequestFactory`` in the type hint, rather

improve factories documentation #161

improve factories documentation #161

Uh oh!

Conversation

dbu commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 4, 2016

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 18, 2016

Uh oh!

xabbuh Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 30, 2016

Uh oh!

sagikazarmark commented Dec 2, 2016

Uh oh!

dbu commented Dec 2, 2016

Uh oh!

Uh oh!

xabbuh Nov 25, 2016 •

edited

Loading