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

Consider changing from having 100 function parameters to an object #601

Closed
adamburgess opened this issue Sep 9, 2022 · 2 comments
Closed
Assignees
Labels
Status: On Hold Explanation in the comments

Comments

@adamburgess
Copy link

SDK you're using (please complete the following information):

  • Version 4.27.0

Is your feature request related to a problem? Please describe.
A lot of the API methods have an insanely long list of parameters.
Here's one in my code: xero.projectApi.getTimeEntries(tenantId, projectId, userId, undefined, undefined, undefined, 1, 450, undefined, undefined, afterDate, beforeDate)
I don't think you can tell me this is ok :)

I feel wary upgrading to new versions as the order of these params may change, and depending on the types it may slip through without a compile error.

Describe the solution you'd like
It's a very large breaking change, though overloading may potentially be possible....

Just use an object. It's much more obvious what is happening, and additional parameters can be added easily without errors.

xero.projectApi.getTimeEntries({
    tenantId,
    projectId,
    userId,
    page: 1,
    pageSize: 450,
    dateAfterUtc: afterDate,
    dateBeforeUtc: beforeDate
});

tenantId could be separate from the object.
Additionally, you could split the options into two parts, url parameters (in this case, projectId) and the POST body.

Describe alternatives you've considered
Some IDEs such as VS Code have a feature called "inlay hints" that write the parameter name directly inline in the call.
This would solve the readability problem, but updates still would be error prone...

@RettBehrens RettBehrens added the Status: On Hold Explanation in the comments label Sep 13, 2022
@RettBehrens
Copy link
Contributor

I like this idea but as you point out it's a large breaking change. We'll place this on hold with plans to explore implementing with the next breaking change

@tnzzz
Copy link
Contributor

tnzzz commented Sep 20, 2022

Hi @adamburgess, just wanted to let you know we're looking into this now. But as Rett said, we'll be implementing this with the next breaking change.

@tnzzz tnzzz self-assigned this Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: On Hold Explanation in the comments
Projects
None yet
Development

No branches or pull requests

3 participants