docs(operators): Add documentation for bufferCount #239
Conversation
Codecov Report
@@ Coverage Diff @@
## master #239 +/- ##
=========================================
Coverage ? 77.14%
=========================================
Files ? 15
Lines ? 175
Branches ? 7
=========================================
Hits ? 135
Misses ? 40
Partials ? 0Continue to review full report at Codecov.
|
Add documentation for bufferCount to close ReactiveX#110
|
|
||
| const clicks$ = fromEvent(document, 'click'); | ||
| const buffered$ = clicks$.pipe( | ||
| map(e => {return {X: e.clientX, Y: e.clientY};}), |
There was a problem hiding this comment.
Not really part of the bufferCount operator, but it might be a better idea to remove the map operator and use the fromEvent's selector argument.
const clicks$ = fromEvent(document, 'click', e => {return {X: e.clientX, Y: e.clientY};});
Nitpick:
Personally, I'm not a fan of using single line functions like this:
e => {return {X: e.clientX, Y: e.clientY};}
What about:
const clicks$ = fromEvent(document, 'click', e => ({X: e.clientX, Y: e.clientY})); ?
There was a problem hiding this comment.
I used that single line format because the curly braces for creating the object would throw errors from the compiler. I had no idea you could use the parentheses to solve that 👍
And I'm used to the map function to select the correct parts of the object, but for clarity I actually prefer your solution...
so thanks @frederikprijck 👍 !
|
|
||
| const clicks$ = fromEvent(document, 'click'); | ||
| const buffered$ = clicks$.pipe( | ||
| map(e => {return {X: e.clientX, Y: e.clientY};}), |
There was a problem hiding this comment.
same remark as below (regarding both the selector argument and the single-lined function).
| import { fromEvent } from 'rxjs/observable/fromEvent'; | ||
| import { map, bufferCount } from 'rxjs/operators'; | ||
|
|
||
| const clicks$ = fromEvent(document, 'click'); |
There was a problem hiding this comment.
I see other source code not using the $ suffix. @ladyleet Is there any guideline for the docs? I think it's a good idea to use the same approach for every example. I'm a fan of using the suffix, just not sure what the docs guidelines are.
There was a problem hiding this comment.
I am generally don't use $ - but I will look through docs/discuss and update about the guidelines and document them.
There was a problem hiding this comment.
I like to use the $ as a suffix as well (as you can see) but I value consistency in coding higher... So I'll change the code to follow the other existing examples for now and we'll see later if we ever want to refactor it!
| 'operatorType': 'transformation' | ||
| name: 'bufferCount', | ||
| operatorType: 'transformation', | ||
| signature: ` bufferCount(bufferSize: number, startBufferEvery: number): Observable<T[]>`, |
There was a problem hiding this comment.
Furthermore we should genrally discuss whether using generics in method signature or not. @ashwin-sureshkumar, @ladyleet , @btroncone
There was a problem hiding this comment.
@ashwin-sureshkumar @jwo719 @ladyleet Let's leave out per #196 (comment)
There was a problem hiding this comment.
So please remove generic
Added example console output as a comment on the examples for jsbin as well as for inline examples. The package.json has a reverted angular-cli dependency so it is only defined in one PR from now on!
DiedrikDM
force-pushed
the
docs-transformation-bufferCount
branch
from
78bb829 to
3e0f360
Compare
| 'operatorType': 'transformation' | ||
| name: 'bufferCount', | ||
| operatorType: 'transformation', | ||
| signature: ` bufferCount(bufferSize: number, startBufferEvery: number): Observable<T[]>`, |
There was a problem hiding this comment.
So please remove generic
The comment containing the expected output needs to be put below the subscription. I have also updated the method signature to remove the generic type.
DiedrikDM
force-pushed
the
docs-transformation-bufferCount
branch
from
b2baf0d to
ebfd33c
Compare
Add documentation for bufferCount to close #110