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

Use named parameters in API requests #95

Merged
merged 1 commit into from Mar 26, 2020
Merged

Conversation

@atoomic
Copy link
Contributor

atoomic commented Mar 18, 2020

This commit is mainly here to open a discussion,
I think it would make sense to be able to use the urls as described in the GitHub API documentation

example: "/repos/:owner/:repo/contents/:path?ref=:ref"
or /orgs/:org/memberships/:username

then when calling a function being able to provide a hash reference

something like this
$Net::GitHub::V3->new(...)->org->membership( { org => 'perlchina', username => 'fayland' } )

note that we can reserve two name arguments for the args or either use an additional hash ref

The commit attached to this PR should not be merged and is mainly there to illustrate the idea.
If you like the idea, I can provide a tidy PR where we could automatically detect if we are called using a hashef or not. So the final user can still use the old syntax, while we start providing the updated one

lib/Net/GitHub/V3/Query.pm Outdated Show resolved Hide resolved
@atoomic atoomic force-pushed the atoomic:named-parameters branch 2 times, most recently from dc9b66e to 50852b9 Mar 18, 2020
Add documentation for get_content and allow
arguments being passed to a GET request.

This is allowing us to do a slow convertion
of dynamic method to v2 of __build_methods
and using the same syntax as the GitHub Doc.
@atoomic atoomic force-pushed the atoomic:named-parameters branch from 50852b9 to f67a10e Mar 19, 2020
@atoomic atoomic changed the title [Do Not Merge] Idea to use named parameters in API requests Use named parameters in API requests Mar 19, 2020
@atoomic

This comment has been minimized.

Copy link
Contributor Author

atoomic commented Mar 19, 2020

@fayland I resolved the conflicts with master and this is ready for a review
would appreciate your feedbacks on it
thanks

@fayland

This comment has been minimized.

Copy link
Owner

fayland commented Mar 20, 2020

it seems working I think. I'll try to review this weekend and make a new release. thanks

@atoomic

This comment has been minimized.

Copy link
Contributor Author

atoomic commented Mar 23, 2020

@fayland did you had any chance to look at it?
thanks

@fayland

This comment has been minimized.

Copy link
Owner

fayland commented Mar 23, 2020

Sorry I forgot to submit the review on the weekend. just one question and please take a look. thanks

@atoomic atoomic requested a review from fayland Mar 24, 2020
@fayland fayland merged commit d99d4af into fayland:master Mar 26, 2020
13 checks passed
13 checks passed
perl (5.30)
Details
perl
Details
perl (5.28)
Details
perl (5.26)
Details
perl (5.24)
Details
perl (5.22)
Details
perl (5.20)
Details
perl (5.18)
Details
perl (5.16)
Details
perl (5.14)
Details
perl (5.12)
Details
perl (5.10)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fayland

This comment has been minimized.

Copy link
Owner

fayland commented Mar 26, 2020

new version released. thanks

@atoomic

This comment has been minimized.

Copy link
Contributor Author

atoomic commented Mar 26, 2020

thanks a lot feel free to ping me if any issues come
nicolas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.