Skip to content
This repository has been archived by the owner on Sep 29, 2022. It is now read-only.

DRF-channels blog post #294

Merged
merged 11 commits into from Dec 12, 2018
Merged

DRF-channels blog post #294

merged 11 commits into from Dec 12, 2018

Conversation

wlonk
Copy link
Contributor

@wlonk wlonk commented Dec 7, 2018

I welcome feedback on this!

Copy link
Member

@stacyk stacyk left a comment

Choose a reason for hiding this comment

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

I haven't ran this yet, but nice article! Left some suggestions, but pretty minor things.

content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
serializers.py
views.py

As a side note, for something that I'm not distributing as a library, I
Copy link
Member

Choose a reason for hiding this comment

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

This makes me think we should add a "side note" type of style to our pattern library. Not now, but in a far future story :)

Copy link
Member

Choose a reason for hiding this comment

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

content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
Copy link
Member

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

This is good!

  • At a certain point, it becomes large code-blocks, with most of the commentary in code-comments. I can see advantages and disadvantages to that approach – more integrated, but also feels more monolithic.
  • Overall there's a strong tone that "you probably already know most of this". I might try to pull that back a bit - a more subtle/tonal form of the best-practice against using "just" or "simply".

content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

👍

content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
content/2018/12/10/channels-and-drf.rst Outdated Show resolved Hide resolved
@wlonk
Copy link
Contributor Author

wlonk commented Dec 10, 2018

@mirisuzanne I appreciate those two points. I've tried to address both, and would be interested in your take on whether I succeeded.

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

Aside from an image, this LGTM!

@wlonk
Copy link
Contributor Author

wlonk commented Dec 10, 2018

@jgerigmeyer clearly I meant to include this image: http://www.lachanelphile.com/wp-content/uploads/2012/10/No5_EXTRAIT_02.jpg

@jgerigmeyer
Copy link
Member

@wlonk I wondered!

@wlonk
Copy link
Contributor Author

wlonk commented Dec 12, 2018

This is ready to merge and publish whenever anyone else says it is. We should tweak the date before publishing though?

@jgerigmeyer jgerigmeyer merged commit 48d61e5 into master Dec 12, 2018
@jgerigmeyer jgerigmeyer deleted the drf-channels branch December 12, 2018 15:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants