Skip to content

Add a maxQueueSize configuration option - #765

Merged
f2prateek merged 1 commit into
masterfrom
feat/add/max-queue-size
Jul 16, 2018
Merged

Add a maxQueueSize configuration option#765
f2prateek merged 1 commit into
masterfrom
feat/add/max-queue-size

Conversation

@fathyb

@fathyb fathyb commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

Ref: LIB-442

@fathyb
fathyb requested a review from f2prateek July 6, 2018 14:52

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did this to avoid an useless cast from NSUInteger (long on 64bits) to int

@f2prateek f2prateek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice - can we add a test case for when a custom queue size is set?

@codecov-io

codecov-io commented Jul 11, 2018

Copy link
Copy Markdown

Codecov Report

Merging #765 into master will increase coverage by 4.43%.
The diff coverage is 93.1%.

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
+ Coverage      81%   85.43%   +4.43%     
==========================================
  Files          54       52       -2     
  Lines        2785     2616     -169     
==========================================
- Hits         2256     2235      -21     
+ Misses        529      381     -148

@fathyb

fathyb commented Jul 11, 2018

Copy link
Copy Markdown
Contributor Author

@f2prateek pushed a test

@fathyb
fathyb force-pushed the feat/add/max-queue-size branch from 03a00f0 to 4b05b45 Compare July 12, 2018 10:48
@f2prateek
f2prateek force-pushed the feat/add/max-queue-size branch from 4b05b45 to 15eed6a Compare July 12, 2018 21:24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what if somebody sets maxQueueSize to 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch! I guess we have to choose from one of these behaviours (ordered with what I think is best first) :

  1. forbid using maxQueueSize = 0 and throw an error in maxQueueSize setter if 0 is set
  2. when maxQueueSize == 0 ignore all events
  3. when maxQueueSize == 0 behave like maxQueueSize == 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Option 4: maxQueueSize == 0 should behave like a no-op, i.e. use the default setting.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Option 5: Document that this must be > 1, and when maxQueueSize == 0 the behaviour is unspecified. I think this is reasonable for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we also document the default value here?

@fathyb

fathyb commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

Pushed improved docs @f2prateek

@f2prateek

Copy link
Copy Markdown
Contributor

hey @fathyb - think we might need a more complicated rebase here. I'll re-assign this back to you for now!

@fathyb
fathyb force-pushed the feat/add/max-queue-size branch from 7e9a433 to a507953 Compare July 16, 2018 19:25
@fathyb
fathyb force-pushed the feat/add/max-queue-size branch from a507953 to 51e00ab Compare July 16, 2018 19:43
@f2prateek
f2prateek merged commit 7331756 into master Jul 16, 2018
@f2prateek
f2prateek deleted the feat/add/max-queue-size branch July 16, 2018 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants