Skip to content

Conversation

@yschimke
Copy link
Member

No description provided.

@NiteshKant
Copy link
Contributor

@yschimke I replied with my thoughts on issue #212

I think semantically we should stick to isValid() doing what it does today but initializing the supplier with -1 and 0 makes sense. Can you revert the isValid() change?

Also, I think this class should be pkg-private and isValid() should perhaps be named isBeforeCurrent() (or some better name)? If you want you can make those changes too 😄

Copy link
Contributor

@NiteshKant NiteshKant left a comment

Choose a reason for hiding this comment

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

Thanks @yschimke ! I will pull this in once you address my comments.

@yschimke
Copy link
Member Author

yschimke commented Jan 3, 2017

So I don't think I should fully revert the isValid changes, since I still think its flipped logically. For an unknown stream id, I still think you only want to log when the stream id is negative or greater than the last issued.

Also the class is used outside the package so I can't make the other change without refactoring.

@NiteshKant
Copy link
Contributor

Also the class is used outside the package so I can't make the other change without refactoring.

Duh, I didn't notice the outside package usages, sorry about that.

Thanks @yschimke !

@NiteshKant NiteshKant merged commit 272401b into rsocket:0.5.x Jan 3, 2017
@yschimke yschimke deleted the stream_ids branch May 6, 2017 06:58
ilayaperumalg pushed a commit to ilayaperumalg/rsocket-java that referenced this pull request Dec 26, 2017
Solve SETUP_ERROR confusion: rsocket/rsocket#191
Exactly specify what error codes are applicable any time an ERROR frame is referenced.
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.

2 participants