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

Version 5 #100

Open
cgratigny opened this issue Aug 21, 2017 · 12 comments · May be fixed by #114
Open

Version 5 #100

cgratigny opened this issue Aug 21, 2017 · 12 comments · May be fixed by #114

Comments

@cgratigny
Copy link

Do you have any active plans to update this to work with version 5?

@jarthod
Copy link
Collaborator

jarthod commented Aug 22, 2017

No active plan on my side, maybe @apurvis? Of course if you want to give it a go that could be an awesome PR :)

@cgratigny
Copy link
Author

Thanks @jarthod. We'll want to start using V5 in June of 2018, so I'll check in. We would be happy to contribute to this, as in June 2018 V4 will start losing support.

@apurvis
Copy link
Collaborator

apurvis commented Aug 28, 2017

i think the playtestcloud people have a fork that is starting to support v5, though i'm not sure where it is: https://github.com/playtestcloud/survey-gizmo-ruby

@apurvis
Copy link
Collaborator

apurvis commented Aug 28, 2017

@cgratigny i had a suggestion for the playtestcloud people over on this commit: playtestcloud@c582719#commitcomment-20474803

i have perused their changes and they look promising though i've never tested them or anything; not sure it's a pure refactor (AKA no functional changes) or if there are real changes hiding amongst the refactor.

it's particularly hard to tell because they changed the whitespace in all the files, which makes it basically impossible to read the diffs.

@peret
Copy link

peret commented Sep 4, 2017

Hey, Peter here from PlaytestCloud. We might be able to provide a PR for this at some point, but I can't make any guarantees to if or when this will happen.
We are actively using our fork with v5 support and it works well enough. There are some functional changes as well, most notably the possibility to configure/use multiple accounts/API keys in the same app. Which, if I recall correctly, was why we forked in the first place...

@apurvis
Copy link
Collaborator

apurvis commented Sep 5, 2017

seems like maybe @cgratigny could take @peret 's branch and make it into a master PR if @cgratigny is willing to deal with whatever niggling little details remain unresolved

@cgratigny
Copy link
Author

Picking this back up. @peret am I correct in the master branch that you have is using V5 of the API? We are delayed in needing to move to V5.

https://github.com/playtestcloud/survey-gizmo-ruby/tree/master

@peret
Copy link

peret commented Jul 15, 2019

Yes, that is correct.

@peret
Copy link

peret commented Dec 23, 2019

I started working on this again and hope to get a proper PR ready during the holidays. I'm wondering though, what the requirements are in terms of backwards compatibility. I guess I can see three ways of approaching this:

  1. Replace all fields with what's there in the v5 API, essentially dropping support for v4 fields (although there's some overlap, obviously). This would therefore be a breaking change.
  2. Mixing both versions in the same class, essentially just adding fields that are new in v5 and leaving the rest untouched. I get the feeling that this is how it works right now with v3 backwards compatibility?
  3. Introducing separate modules for the different API versions and split the fields up.

I think 1 and 3 would be clean solutions, with Option 1 being easier to implement.

I would love to get some opinions on what would be the best approach :)

@jarthod
Copy link
Collaborator

jarthod commented Dec 26, 2019

Thanks for getting back to this @peret, this is much appreciated.

For me the best would be to keep support for v4 and v5 (we can drop v3 if it cleans stuff but otherwise we can keep it) with v5 being the default so it keeps working as before for people already configured for v4. This can be achieved with option 2 or 3, 3 looks cleaner indeed (If it can be made without changing the public API) but probably requires much more changes/time also, no?

Option 1 could be acceptable IMO only if the changes are small and we provide a comprehensive list of things to change otherwise people will simply never upgrade.

@peret
Copy link

peret commented Dec 26, 2019

Great, thanks for the feedback! I'll see if I can find a way to have multiple versions of the API classes and make them accessible through the same public API.

@peret peret linked a pull request Apr 24, 2022 that will close this issue
@peret
Copy link

peret commented Apr 24, 2022

I finally found the time to prepare a PR to add v5 support 👍

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 a pull request may close this issue.

4 participants