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

Change tags to on operations to group methods differently #134

Open
SidneyAllen opened this issue Feb 5, 2020 · 2 comments
Open

Change tags to on operations to group methods differently #134

SidneyAllen opened this issue Feb 5, 2020 · 2 comments
Assignees
Labels
Status: In Discussion Tag individuals in the comments you wish to discuss issue with Type: Feature Request

Comments

@SidneyAllen
Copy link
Contributor

This started as an issue on our PHP SDK
XeroAPI/xero-php-oauth2#53

To summarize, at this time all Accounting endpoints are tagged "Accounting" in the yaml file and when code is generated all operations (methods) are put into the same AccountingApi class. With so many endpoints and actions you can perform the AccountingApi class is large.

Changing how operations are tagged is fairly simple, but will have ripple effects across SDKs, documentation and sample code. So, this is not a change that we will be implementing at this time but will consider along with other requests that result in major refactoring of assets.

I'd like to hear what ppl think about this idea? Do you like creating a singleton (AccountingApi) and working with that or would you like to create an instance of AccountApi, ContactApi, InvoiceApi, etc?

Alternatively, we could group based on scope and not endpoint.
https://developer.xero.com/documentation/oauth2/scopes

  • transactions, reports, settings, contacts, journals

In the above scenerio 'TransactionsApi class would have all the banktransactions, banktransfer,creditnote, invoice, overpayment, payment, etc together.

Look forward to hearing ppl's thoughts.

@SidneyAllen SidneyAllen added the Status: In Discussion Tag individuals in the comments you wish to discuss issue with label Feb 5, 2020
@hailwood
Copy link

hailwood commented Feb 6, 2020

I love the idea of injecting just a ContactApi etc.

As a potential solution that could satisfy both camps, could the API also generate an AccountingApi class that just had methods to return an instance of each of the apis?

Then you can choose to inject the individual apis, or inject the AccountingApi and call $accountingApi->contacts()->getContacts(....) etc? With the contacts method simply returning an instance of the ContactApi created with the Guzzle client and Xero Configuration class passed to the AccountingApi constructor?

I don't think we should group by scope as it then becomes a bit confusing to work out what we need to inject to access what.

@judgej
Copy link
Contributor

judgej commented Mar 10, 2021

Each endpoint can have multiple tags, so accounting could group everything in the Accounting API, and then additional tags could group other collections of endpoints. I could imagine there would be some overlap. Being able to run a small client with a collection of related enpoints to perform a task that needs just those enpoints, I'm sure will be useful. Having a single client to span the full range of endpoints is also very useful for tasks that need it (e.g. we query bank accounts, transactions, invoices and clients all in one job - these things are all very closely related.

So I agree with @hailwood - a tag for everything, and tags for groups of related functions. When generating code, you can always choose one or the other (or both) by generating against chosen tags only.

The way the PHP clients are built, I think is very close to how the clients for other languages would be built, at least when they are generated through the https://github.com/OpenAPITools/openapi-generator tool. All the languages are given the same data structure to work from, and that colours the structure of the code that is generated.

The PHP code is quite long (an understatement) because of the way the templates build it "long-hand" with very few shared classes. Two points to note here: firstly PHP doesn't care much abut the length of these classes. Secondly it can be shortened somewhat with custom templates. The OpenAPI builder v5 makes it even easier to add your own custom support/utility classes in the generated code to reduce the size even more.

Just throwing my 2p into the ring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Discussion Tag individuals in the comments you wish to discuss issue with Type: Feature Request
Projects
None yet
Development

No branches or pull requests

3 participants