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

Remove "context" #54

Closed
wants to merge 6 commits into from
Closed

Remove "context" #54

wants to merge 6 commits into from

Conversation

mariusandra
Copy link
Contributor

We ignore it and never use it, so let's remove. This is a remnant from the early life of this library.

@neilkakkar
Copy link
Contributor

lol, was wondering about this today, I guess safe to remove, will do so 👌

@haacked
Copy link
Contributor

haacked commented Feb 5, 2025

I resolved the merge conflicts.

I support this change, but I do have a concern that it might break existing customer code that calls capture. For example, if someone were to have code like this:

client.capture("some_distinct_id", "some-event", None, None, datetime.now(tzutc()), None, None, True)

When we remove the context argument, this call becomes invalid. Should we consider adding a deprecation warning log message before yanking this argument? Or is it very unlikely someone is calling this method with this many positional arguments. Hopefully folks are using named arguments once they get toward the end of the argument list. 😄

@mariusandra
Copy link
Contributor Author

Deprecating might indeed be safer, as we don't want to break people doing a patch or minor update. It'll remain a no-op like now, but at least it'll be clear that it's not to be used. Then we can remove it altogether after a few months or so...

@haacked
Copy link
Contributor

haacked commented Feb 5, 2025

I can make that change if you'd like. I'll see if I can find other examples for how we deprecate things.

@haacked
Copy link
Contributor

haacked commented Feb 10, 2025

I opened a PR #182 to deprecate context. I'll close this PR and log an issue to remove context some time later.

@haacked
Copy link
Contributor

haacked commented Feb 10, 2025

Issue created here: #183

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.

4 participants