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

added getSessionId #165

Closed
wants to merge 1 commit into from
Closed

added getSessionId #165

wants to merge 1 commit into from

Conversation

colin-persona
Copy link

💡 Motivation and Context

I need your sessionId to help correlate your events with my own server events.

#164

💚 How did you test it?

In my app.

📝 Checklist

  • [ X] I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

NOTE: The code I wrote is this:

    @objc public func getSessionId() -> String {
        if !isEnabled() {
            return ""
        }
        
        return sessionLock.withLock {
            return sessionId ?? ""
        }
    }

which would have been better as this (return optional, use guard), but I did the above to match your other similar functions...

     @objc public func getSessionId() -> String? {
        guard isEnabled() { return nil }
        return sessionLock.withLock {
            return sessionId
        }
    }

Anyway, this will help me, thanks!

@colin-persona
Copy link
Author

I didn't bump version or edit changelog - I can if you'd like, but I figured that'd be you folks' department. It'd be nice for this to get deployed with new version so spm would pull it down right away...

@colin-persona
Copy link
Author

Also, not quite sure which tests you'd want... brand new one? Shoehorn into "identify sets distinct and anon Ids"? How would it be done?

Comment on lines +179 to +187
@objc public func getSessionId() -> String {
if !isEnabled() {
return ""
}

return sessionLock.withLock {
return sessionId ?? ""
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@objc public func getSessionId() -> String {
if !isEnabled() {
return ""
}
return sessionLock.withLock {
return sessionId ?? ""
}
}
@objc public func getSessionId() -> String? {
if !isEnabled() {
return nil
}
return sessionLock.withLock {
return sessionId
}
}

@marandaneto
Copy link
Member

@colin-persona thanks for the PR.
the guard option is a nice idea, I can refactor all methods later.
getSessionId can return nil since it's not guaranteed that the session feature is enabled.
feel free to add a changelog entry, just put it under Next and follow the others as an example.
You can add a test on PostHogSDKTest since the sessionId is created during SDK init, you just need to guarantee that the getSessionId returns non-nil.

@marandaneto
Copy link
Member

@colin-persona you can run make format (commit and push if needed) and later make lint to guarantee that CI is happy, and of course make test if you add a test.

@marandaneto marandaneto mentioned this pull request Aug 27, 2024
4 tasks
@marandaneto
Copy link
Member

closed in favor of #170

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