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

[WIP] Add option to client for SDK specific identifier #713

Closed
wants to merge 1 commit into from

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented Nov 27, 2018

This is a WIP to propose an alternative approach to #710. I have to add a lot of stuff (tests, same stuff in the client builder) before proceeding, but I would like to ask if this approach seems good.

@Jean85 Jean85 added this to the Release 2.0 milestone Nov 27, 2018
@Jean85 Jean85 self-assigned this Nov 27, 2018
@Jean85 Jean85 requested review from HazAT and ste93cry November 27, 2018 23:04
@HazAT
Copy link
Member

HazAT commented Nov 28, 2018

Hmm, I am not sure about this.
I don't really want that the user is able to overwrite the sdk identifer when initalizing a Client, it seems odd. Also we need to be able to change the version.

What about if we do something similiar like in #710 but instead of changing ClientBuilder so deeply
we are able to pass a ClientClass into ClientBuilder::create()->getClient($class)?

I really don't want that the sdk identifer / version is a option the user can set.
I am not seeing any other simple way than inheritance of Clients.

@Jean85
Copy link
Collaborator Author

Jean85 commented Nov 28, 2018

Client inheritance would still open the possibility of extension to the end user, so we would still not be able to block that..

@Jean85 Jean85 mentioned this pull request Nov 29, 2018
6 tasks
@HazAT
Copy link
Member

HazAT commented Dec 7, 2018

I think we can close this right?

Edit: Please delete the branch if we don't need it anymore.

@HazAT HazAT closed this Dec 7, 2018
@Jean85 Jean85 deleted the pass-sdk-integration-identifier branch December 7, 2018 15:15
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