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

feat: isolated client class to allow for parallel usage + extension mocking #228

Merged
merged 2 commits into from
Jun 12, 2023

Conversation

daniel-sc
Copy link
Member

Hi all,

the issues with the static Podio class were nagging me since long - with this change, we should have better architecture. I admit that this is a large code change and of course a breaking change for users, but the following improvements are worth it from my perspective:

  • Easy handling of multiple/parallel logins/authentications (simply create multiple instances of PodioClient)
  • Easy extension (you can subclass PodioClient to alter behavior)
  • Easy mocking/stubbing (now it would be feasible to unit test a "save" method..)

Please take this PR as a basis for discussion, if you think this could be done better or should not be done at all, I'm happy about any feedback!

closes #90
BREAKING CHANGE: replace static Podio with instantiable PodioClient

… mocking

BREAKING CHANGE: replace static Podio with instantiable PodioClient
@daniel-sc
Copy link
Member Author

@carlfredrikhero @rafaelmb @bajarang-agarwal @timosillander @dylannadon anyone any inputs? (as noted before, I'm happy to deal with critical feedback ;) )

Copy link
Member

@rafaelmb rafaelmb left a comment

Choose a reason for hiding this comment

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

LGTM!

@bajarang-agarwal
Copy link
Contributor

@daniel-sc
Thank you for sharing your idea and making this large change. I agree that having a static class is really not good and the new approach is going to improve things a lot. You really spent a long time to get this right.

This is a major breaking change and it will impact a large number of users. I am fully not 100% sure whether we should make a big breaking change or not. So I am happy to hear the feedback from other community members.

I have not tested the PR but in review, this looks good to me.

@daniel-sc
Copy link
Member Author

@bajarang-agarwal Completely agree!

To facilitate a smooth transition we could make a beta release. Then we and other users could test and provide feedback on problems?

I'd suggest to first merge and then create the beta release (from master). But I'm not 100% sure, what do you think?

Should we wait some more, or should we proceed?

@bajarang-agarwal
Copy link
Contributor

@daniel-sc
I completely agree with the beta release, you can proceed with merging and making the beta release.

@daniel-sc daniel-sc merged commit a576ef8 into master Jun 12, 2023
@daniel-sc daniel-sc deleted the 90-non-static-podio-class branch June 12, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-static Podio class
3 participants