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

add getter for session transport protocol #55

Closed
wants to merge 1 commit into from

Conversation

Morfent
Copy link

@Morfent Morfent commented Dec 20, 2016

This is useful when diagnosing issues with SockJS that may be related to what
kind of protocol a connection is using.


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 94.839% when pulling 76d8609 on Morfent:transport into 1f275fb on igm:master.

@Morfent
Copy link
Author

Morfent commented Dec 20, 2016

Added unit tests

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 95.484% when pulling 5748021 on Morfent:transport into 1f275fb on igm:master.

@FZambia
Copy link
Collaborator

FZambia commented Dec 20, 2016

Hi, why not just call path.Base(s.Request().URL.Path) in you app code? It's not difficult and this way we don't introduce new method to Session interface.

@Morfent
Copy link
Author

Morfent commented Dec 20, 2016

It's not difficult, but it won't always be clear to others reading that code that the end of the URL is going to be the protocol unless they're pretty familiar with SockJS. I think it'd also help with people transferring code from sockjs-node since protocols are just properties on session objects in that package.

@FZambia
Copy link
Collaborator

FZambia commented Dec 20, 2016

Sounds reasonable 👍

This is useful when diagnosing issues with SockJS that may be related to what
kind of protocol a connection is using.
@igm
Copy link
Owner

igm commented May 31, 2018

not sure if this PR is still relevant and demanded. I generally prefer not to change interfaces.


Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.


sockjs/session.go, line 227 at r1 (raw file):

func (s *session) Protocol() string {
	protocol := path.Base(s.req.URL.Path)

would it be sufficient to put session as a parameter instead of receiver?

func Protocol(s Session) string {
	protocol := path.Base(s.Request().URL.Path)
	return protocol
}

This does not change session interface


Comments from Reviewable

@igm
Copy link
Owner

igm commented Apr 26, 2020

fixed in v3

@igm igm closed this Apr 26, 2020
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.

None yet

4 participants