-
Notifications
You must be signed in to change notification settings - Fork 522
New package: RestClient v1.0.0 #129723
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
New package: RestClient v1.0.0 #129723
Conversation
c91e2d1 to
ac24d94
Compare
|
Hello, I am an automated registration bot. I help manage the registration process by checking your registration against a set of AutoMerge guidelines. If all these guidelines are met, this pull request will be merged automatically, completing your registration. It is strongly recommended to follow the guidelines, since otherwise the pull request needs to be manually reviewed and merged by a human. 1. New package registrationPlease make sure that you have read the package naming guidelines. 2. AutoMerge Guidelines are all met! ✅Your new package registration met all of the guidelines for auto-merging and is scheduled to be merged when the mandatory waiting period (3 days) has elapsed. 3. To pause or stop registrationIf you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text Tip: You can edit blocking comments to add |
ac24d94 to
5b5aa4c
Compare
|
What is the reasoning behind the name? I would expect to get a RestClient from something like this package - except I would I expect it to be called RestClients (plural). It seems there is no client type in the package? [noblock] |
|
[noblock] Seems fine to me. The whole package is a REST client, and the well-documented API is exactly what you would want to talk to a REST interface. Maybe think of it as a Singleton. That makes the singular name absolutely appropriate. |
|
[noblock] Thanks for the feedback both. I expect REST clients for individual services to be named after the services, and not include "REST" in the name at all. If say the package provided a |
|
I agree that REST clients should in general be named after the services, but they could include REST in their name to disambiguate among, e.g. a REST client and a SOAP client for the same service. I am very much in the target audience for a REST client package, but only after digging down to the tutorial (1) do I kind of understand what RestClient tries to achieve. From what I can grasp the intended usage is to create a module, e.g. This does not seem very natural. I would expect a REST Client package (claiming that name in the General registry) to provide functionality that would allow the user to create a (1): From the README and the Introduction, I was mostly left with claims ("Simplify", "concise", "capable", "absorb as much of the common complexity ... as possible", "sensible OOTB behaviour, "minimal setup") - and no explanation from what RestClient actually provides (macros for defining a REST API client). [noblock] |
|
[noblock] It sounds like you're expecting that a package that helps you write REST clients would do so in a structure that involves creating a type for a particular API/client? I must admit I don't see why that would necessarily be the case. A module/package seems much more natural to me.
Writing a command that turns an OpenAPI definition into a package using
That's surprising to hear, I had hoped that these bits of the readme would clarify that:
If there are specific details that you think it would have been helpful to see earlier, it would be great if you could highlight them so I could improve the readme/docs. |
|
For the documentation, an example front and center showing actual use of an API via RestClient would help a lot: When I dug into the tutorial yesterday and accidentally skimmed past what What I was thinking was
😊 This is of course ridiculous, and I should have read the tutorial more thoroughly, but I think a concrete usage example in the README would have avoided the confusion. Regarding the RestClient API, one source of confusion is the naming of the Another source of confusion is macros - and, in particular, macros with opaque names/functionality - e.g. what does I think the macros are a bit too magical - I guess the Regarding the type vs module discussion: With the premise that the client API is defined via macros, it makes sense, of course, to define a module. And vice versa, without this premise, I think the module would not be necessary. I think it might be worthwhile to, at least superficially, explore how the majority of APIs are defined and consumed today, via OpenAPI definitions: It would be very nice with a way to go directly from a YAML/JSON OpenAPI definition file and to a Julia client API. And it might indeed make sense to define such an API via macros, e.g., something like As an added argument for the plural, RestClients, naming, it also would not block consumers from defining a RestClient type (whatever reason they might have for doing this). [noblock] |
|
[noblock] Thanks for the feedback @stemann!
I had hoped that the example in the readme might do this? It's complete and fully-functional. Do you find it a bad example, or is there something else about it that stops it from being good enough? Or are you specifically suggesting that I put a little complete example like it at the start of a tutorial?
Thanks for mentioning this specific confusion, do you think it would help if I clarified that line with something like: julia> deck = DeckAPI.new() # Create a new 'deck' using the API
Hmm, I'm open to renaming that, but I don't think
It is fully possible to benefit from the library without using macros. I try to avoid making them seem to magical and actually show what they do by implementing the first endpoint in the tutorial without using the
Even without macros, I still think it makes sense. What would the type-based approach involve? Doing something like
This largely consists of code generation, which https://github.com/JuliaComputing/OpenAPI.jl does, I'm just want a library that (1) works for non-OpenAPI REST APIs (2) includes OOTB support for rate limiting and caching (3) produces nice code to read. It's not likely there will be an
I suppose, I can't see this being a major motivation though. I'll poll Slack or somewhere again to see if I draw on a few more second opinions regarding the namee. |
Just adding this one line would help (a lot).
Do you intend for the I'm glad you mention auth. as that is definitely a key ingredient in calling APIs. I did not see any mentions of authentication/authorization earlier. How to inject an API key into a RestClient API (client)?
I skimmed past that part (too) :-) A bit out of at-keyboard time right now, but might check again later...
Exactly, It might, in particular, make sense if you would like to talk to two services with the same API: That is a concrete use case from my past - migrating e.g. from GitLab.com to a self-hosted GitLab instance.
I don't quite get what you mean? The suggested The benefit wrt. OpenAPI.jl being to have the code generation done by an in-language Julia macro instead of Java-generated code (including nicer stack traces when stuff breaks). Just a summary at this point: Wrt. to this PR, my suggestion is to go for
[noblock] |
|
[noblock]
Thanks for this feedback, I'll be making this change.
It's not intended to provide all of the information needed to call an API, just centralise some of the common configuration (base URL, API key, etc.)
That's not explicitly covered in the docs (yet), you have to read between the lines a little bit.
For cases like this, you can just create a
I think this sort of code-generation (involving reading the filesystem at compile-time) goes beyond what's sensible for a macro. Regarding the package name, I don't think Regarding Lastly, regarding setting the version to < 1.0, sem-ver v1 doesn't mean "finished/feature-complete", it just means no API breakage till the next major release, and I think that's appropriate for the package at the moment. |
That sounds like pretty much all of the information to me... what are you thinking is missing?
OK (though that sounds definitely pre-1.0 - not having auth).
How would this look? Wouldn't I have to define the API twice? ?
True - you could of course read the file separately and then feed something to the macros. I'm thinking that the content being fed to
My thought here was that if you strip away the macros, there's not that much left in addition to JSON3, StructTypes, HTTP etc.
In any case, please give it some time (measured in days at the very least).
Sure, but so does v0.1 (at least in the Julia/Pkg interpretation of sem. ver.). I know there are vocal opponents to this, but I'm not on-board with the "everything has to start at v1.0" thing. I think v1.0 should at least be "I've considered the proposals in the design phase and consider this to be complete wrt. basic requirements" - this is of course entirely up to you, but my two cents would require authentication and being able to connect to two services with the same API (non-exhaustive list). [noblock] |
The API itself 😛
I'm personally comfortable with better docs/examples coming in minor releases.
No, you'd do this module MyFancyAPI
@globalconfig RequestConfig("https://original.domain")
@endpoint foo(...) ...
endand then to use it somewhere else newconfig = RequestConfing("https://other.domain")
MyFancyAPI.foo(newconfig, ...)
src/requests.jl and src/caching.jl may not be long, but they're certainly not trivial.
Package registration takes a minimum of three days, and unless you add
Neither am I, but to quote myself from earlier "I think [v1.0] is appropriate for the package at the moment". [noblock] |
Right, but I mean, all the information to call the entity a client seems to be there... from my perspective.
Definitely. I just got the impression that auth. is not even supported at this point?
OK! That's a bit surprising. I've tried looking at the documentation again - trying to understand what a config is and where it is used (and passed to endpoints), and I really have a hard time with the API. I might even say I'm having close to the opposite experience of what @goerz phrased: "the ... API is exactly what you would want to talk to a REST interface". Edit: ... Though I do really appreciate that you are tackling this challenge, and I appreciate the clever work that went into this. ... I just have a hard time grasping the API design...
That does indeed look non-trivial. My interpretation might've been pre-judicial (just based on a glance at the
Nice 😊 [noblock] |
There's a dedicated slot for auth to be stored in
The config is exactly five things:
This is not clear in the current docstring, and is one of the docstrings I'll be improving.
That is a pity to hear. Hopefully, the docs improvements that I'm making in response to your comments will help. I imagine examples might also be useful, and in time I could point to a few within the package docs/readme.
Thanks, I'll edit my comments in a bit to unblock this registration seeing as the poll is sticking around ~75% in favour of the singular form, and I'm still not keen for longer forms like |
5b5aa4c to
c20a2b9
Compare
I can also apply an override to “ignore blocking comments”. Let me know if you want me to do that. [noblock] |
Thanks for the offer Michael! @stemann has communicated with me over Slack that he wants to leave his first comment as blocking for another week though (which I'm not thrilled about but also not too fussed about), so that would probably be premature. [noblock] |
|
I added the multiple choice poll that I had requested - I had to add it as an edit to one of my messages, as I had hit a max-three-consecutive-messages limit: https://discourse.julialang.org/t/pre-ann-restclient-jl/123828/27 [noblock] |
|
I've opened a couple of issues in RestClient.jl to split out the API discussion. [noblock] |
UUID: e1389577-bcae-4d0f-b368-61b404b8c94c Repo: https://github.com/tecosaur/RestClient.jl.git Tree: 4b31b3e9ad66b0f2e4db243735790a50817d10ad Registrator tree SHA: d5716b7a540e5fbc43640c2fff2906fe50e9525a
c20a2b9 to
7c2abb2
Compare
|
Friendly poke @stemann 🙂 [noblock] |
|
Thanks for the ping, and sorry for the delay - been away from the keyboard for some days. Just reached out to the web channel on slack (as I had hoped to get input from the JuliaWeb / JuliaServices communities) - if there’s no significantly altering feedback/discussion within 48 hours from now, I will unblock my comment. [noblock] |
|
Unblocking as there has been no further input 😊 [noblock] |
Uh oh!
There was an error while loading. Please reload this page.