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

Split Gopay\Api to other repo #34

Closed
f3l1x opened this issue Apr 30, 2015 · 11 comments
Closed

Split Gopay\Api to other repo #34

f3l1x opened this issue Apr 30, 2015 · 11 comments

Comments

@f3l1x
Copy link
Member

f3l1x commented Apr 30, 2015

I suggest to split Gopay\Api to other repo - Markette/GopayApi.

cons:

  • composer package - for those who would like implement their own library, there is no gopay-api package today
  • 100% tests for Markette/Gopay - at this moment, only API is uncovered
  • developing - you can update GopayApi without changes in Gopay, easy maintain more branches and APIs

What do you think? @hrach @vojtech-dobes @hranicka @haltuf

@vojtech-dobes
Copy link
Contributor

I personally like it - I started this package without API in it, as the API and this library aren't same thing. Of course ideal situation would be if Gopay maintained their API by themselves :).

@haltuf
Copy link
Contributor

haltuf commented Apr 30, 2015

I like it, too, I believe it's a good idea. When doing it, I think it would be fine to remove some inconsistencies in GopaySoap (half of functions is static, half isn't - in official api, all are static)

@f3l1x
Copy link
Member Author

f3l1x commented May 1, 2015

It's a question. Keep it static or transform to something useful. I vote for keep it static. What do you think?

@hranicka
Copy link
Contributor

hranicka commented May 2, 2015

👍 for keep it static (as @haltuf said: "in official api, all are static").

@hranicka
Copy link
Contributor

hranicka commented May 2, 2015

@f3l1x I've created base of the new repository: https://github.com/Markette/GopayApi

It's an official GoPay PHP API from http://help.gopay.com/cs/tema/integrace-2/integrace-platebni-puvodni-brany/php-api-verze-2-5

But namespaced & with typos (oh, it's ugly code).

We should also add some description and tag v1.0 or v2.5? (it's GoPay API 2.5)

Can we then remove Markette\Gopay\Api and replace it with composer-loaded Markette\GopayApi (BC-break, release v3.0)?

@haltuf
Copy link
Contributor

haltuf commented May 2, 2015

I would keep it static. The less changes are made to the official API, the easier it will be to maintain in next versions...
I prefer tagging in agreement with official API, i.e. tag 2.5 - it's more transparent - and keep it synced with future API releases.

Regarding the v3.0 in this repo, maybe we should wait a little bit, as this very change doesn't actually bring anything new for the user. I could imagine, that markette/gopay with functions from #35 could be a candidate for v3.0 (didn't think about it much yet, but I suspect there will be need for some changes that might ALSO introduce another BC break - maybe, maybe not - so why not to do it at once?)

@f3l1x
Copy link
Member Author

f3l1x commented May 2, 2015

I prefer tag same as API version. We could create a milestone 3.0. There could be more changes, for example new dependency gopay-api, PHP 5.5 syntax, new features, more examples.

@hranicka
Copy link
Contributor

hranicka commented May 2, 2015

@f3l1x Why PHP 5.5 syntax in milestone 3.0? 😮 Did you mean 5.4 due to short array syntax etc. or you really need 5.5 features?

Personally I like 5.6, but still prefer 5.4 because of many of our clients have it on theirs hosting.

@haltuf
Copy link
Contributor

haltuf commented May 2, 2015

I agree with @hranicka, unless it has some added value, I would keep the compatibility with lowest possible version. According to the last reports, PHP5.5 is still not very common on production servers.
👍 for the rest, @f3l1x

@f3l1x
Copy link
Member Author

f3l1x commented May 4, 2015

@hranicka @haltuf Sure, PHP 5.4, my mistake.

We've an API repo. https://github.com/Markette/GopayApi

@f3l1x
Copy link
Member Author

f3l1x commented May 14, 2015

Merged #40 .

@f3l1x f3l1x closed this as completed May 14, 2015
@f3l1x f3l1x mentioned this issue Oct 9, 2015
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants