Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document SDLPrioritizedObjectCollection priority lower is 'higher' #143

Closed
joeljfischer opened this issue Mar 26, 2015 · 10 comments
Closed
Labels
best practice Not a defect but something that should be improved anyway documentation Relating to inline documentation
Milestone

Comments

@joeljfischer
Copy link
Contributor

It should retrieve high priority objects first, but instead retrieves low priority objects first.

Because this is used with service numbers, lower numbers indicate higher priority. Since this semantically odd, it should be documented within the class.

@joeljfischer joeljfischer added the bug A defect in the library label Mar 26, 2015
@joeljfischer
Copy link
Contributor Author

@hugh22 @Heath-FoMoCo Are lower numbers higher priority? Because that would mean this is functional then, just semantically weird.

@hugh22
Copy link
Contributor

hugh22 commented Mar 27, 2015

Nope, that looks like an error. The current spec (which I suspect will need changing) is to use the number of the service type as the priority with higher values going first.

@joeljfischer
Copy link
Contributor Author

Okay, thanks, just wanted to make sure

@joeljfischer
Copy link
Contributor Author

I plan to hold off fixing this until unit tests are merged so that I can test the fix properly.

@justinjdickow
Copy link
Contributor

The current specification says, in section 4.

Lower service numbers have higher priority and are sent ahead of higher service numbers.

I don't have an exact reference because I rewrote the protocol spec for GitHub but my wording is

Lower values for service type have higher delivery priority.

@joeljfischer
Copy link
Contributor Author

@justinjdickow @hugh22 So...it's functional then? Just semantically odd? I can just change the test for high priority to be a lower number, and it should pass.

@justinjdickow
Copy link
Contributor

@joeljfischer Assuming the service number is always used as the priority, it would seem it is functionally correct

@joeljfischer
Copy link
Contributor Author

Alright, I'll alter the test then. This should be documented within the SDLPrioritizedObjectCollection class.

@joeljfischer joeljfischer changed the title SDLPrioritizedObjectCollection stores objects in reversed order Document SDLPrioritizedObjectCollection priority lower is 'higher' Mar 27, 2015
@joeljfischer
Copy link
Contributor Author

Alright, I'll alter the test then. This should be documented within the SDLPrioritizedObjectCollection class.

@hugh22
Copy link
Contributor

hugh22 commented Mar 27, 2015

Yeah, that's right. It got turned around so that an RPC can pre-empt video streams.

@joeljfischer joeljfischer added documentation Relating to inline documentation best practice Not a defect but something that should be improved anyway and removed bug A defect in the library labels Mar 30, 2015
@joeljfischer joeljfischer added this to the 4.0.0 milestone Mar 30, 2015
joeljfischer pushed a commit that referenced this issue Apr 20, 2015
Fix regression on return type on `nextObject` from `instancetype` to `id`

Fixes #143
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
best practice Not a defect but something that should be improved anyway documentation Relating to inline documentation
Projects
None yet
Development

No branches or pull requests

3 participants