Closed marcandre closed 5 years ago
Yes, I decided against "promoting" the toplevel params to all endpoint params after some experimenting: it makes endpoint API less clear (like, for deeply nested endpoint, the inspect/describe rendering, to stay true to the real things, should say something like endpoint(my_params, my_parent_params, and_all_other_params_you_can_imagine)
), as well as making vague distinction between "global API param, set once on API obj.creation (like api_key
)" and "local endpoint param, but defined on global level".
The first API that comes to mind is something like RSpec's "shared_contexts":
# on top level, or some namespace level:
shared_params :pagination do
param :page
param :per_page
end
# ^ this definition doesn't attaches params to anything, just defines reusable block
# at some endpoint level:
has_params :pagination
WDYT?
For the interface (for help), a solution would be endpoint(my_params, my_parent_params, **globals)
, so it won't matter how many global options
there are. Or even just **
or **g
for brevity.
In my case, globals are really for most or all calls, so it would be nice to have a way to be DRY about it. Maybe global :foo, {only: [endpoint_names]} {except: [endpoint_names]}?
On Thu, Sep 27, 2018, 14:15 Victor Shepelev, notifications@github.com wrote:
Yes, I decided against "promoting" the toplevel params to all endpoint params after some experimenting: it makes endpoint API less clear (like, for deeply nested endpoint, the inspect/describe rendering, to stay true to the real things, should say something like endpoint(my_params, my_parent_params, and_all_other_params_you_can_imagine)), as well as making vague distinction between "global API param, set once on API obj.creation (like api_key)" and "local endpoint param, but defined on global level".
The first API that comes to mind is something like RSpec's "shared_contexts":
on top level, or some namespace level:
shared_params :pagination do param :page param :per_pageend# ^ this definition doesn't attaches params to anything, just defines reusable block
at some endpoint level:
has_params :pagination
WDYT?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/molybdenum-99/tlaw/issues/18#issuecomment-425192371, or mute the thread https://github.com/notifications/unsubscribe-auth/AACD6mg3jaX43Du8nB66WOMk7fEqiWA-ks5ufRW0gaJpZM4W2CTJ .
Honestly, I don't believe globals
solution adequately represents "what's going on here". For me (if I understand your wishes correctly):
page
param"endpoint(..., page:)
I don't think syntetic "globals" (or however you call it) concept represents those things naturally. What do we really need is: do exactly the same as param :page
in each endpoint will do, but more DRYly.
So, remembering each endpoint ...
produces, in fact, some class, I can think of these ways of express the meaning:
just include some module:
class MyAPIWrapper < TLAW::API
module HasPagination
param :page
end
# ...
endpoint :foo do
include WithPagination
# DSL-y version (I've proposed this above)
shared_definition :pagination do
param :page
end
endpoint :foo do
with :pagination
Contra: with :pagination
still will be repeated everywhere.
modify base endpoint class
class MyAPIWrapper < TLAW::API
class Endpoint < TLAW::Endpoint # somehow now make all endpoints descend from this
param :page
end
# DSL-y version:
extend_endpoints do
param :page
Contra: doesn't provide a clean way to say "but this endpoint don't have page
!", save for some tricks like remove_param :page
inside "this endpoint".
Post-processing of endpoints (it is pretty close in terseness with your globals
, but has less "syntetic" feeling for me):
# AFTER all the definitions of namespaces & endpoints:
each_endpoint { |e|
e.param :page
}
# you can exclude some:
each_endpoint.reject { |e| e.path == 'foo/bar' }.each { |e|
e.param :page
}
# or even...
each_endpoint(path: %r{^(foo/bar)}) { |e|
# in combination with proposal from my (1) above:
shared_definition :pagination do
param :page
end
# can still be included individually, but also:
each_endpoint { |e| e.with :pagination }
WDYT?
Sorry, forgot to reply...
Personally, I know that in my README, I will not list the page
+ per_page
params on each endpoint, I'll add a note somewhere in the section. I like DRY.
For the info in .describe
, it makes sense to show them I guess.
API: 1)
class MyAPIWrapper < TLAW::API
module HasPagination
param :page
end
# ...
endpoint :foo do
include WithPagination
This won't work.
1) param
is not defined within Pagination
. You'll get a NoMethodError
.
2) It's not clear how you'll have the include
do anything useful. include
only brings in the instance methods, it doesn't re-execute stuff. This ain't a SuperModule :smile:
Maybe you're thinking of
class MyAPIWrapper < TLAW::API
module HasPagination
def self.included(base)
base.param :page
end
end
# ...
endpoint :foo do
include WithPagination
?
That isn't very DSL like.
In any case, my endpoints are currently very simple, basically they're mostly endpoint :foo \n desc '...' \n end
, I really do not want to add a line to each of them, whatever that line may be. Not DRY.
2) Not too convincing, IMO
3) I tried that, actually, but it doesn't work (currently).
all_endpoints.each do |ep|
ep.param_set.add(:page, desc: 'page (default 1)')
ep.param_set.add(:per_page, desc: 'results per page (default is `30`, max i
100`)')
end
This doesn't redefine the methods...
I'm still in favor of:
4)
class MyAPIWrapper < TLAW::API
global :page
global :per_page
endpoint :foo do
# has :page and :per_page params
end
# For exceptions, explicit list of globals may be given:
namespace :bar, globals: [:page] do
global :baz
# endpoints will have globals :page and :baz, unless specified otherwise
end
I believe that the above represents clearly exactly and minimally what is going on. Implementation would be very simple too (I'd gladly provide a PR).
About "it wan't work"/"isn't very DSL" comments -- that was pseudocode, not actual code, just to show options: either it is a module, or base class, or addition to each class in a cycle, there are only 3 options.
Let's reiterate one more time, sorry for pickiness, but I still have a feeling of common ground trying to be found.
My points are:
global
, for me, is a wrong both as the name and the concept. There is nothing "global" in the idea that "each endpoint has property X" ("global" for me reads "this whole API, globally, has parameter..." which is totally not what's going on).# option A: predefined mixin, manually included in each relevant endpoint later:
shared_definition :with_pagination do # exact name is discussable, but should be clear
param :page
param :per_page
end
# ...options of usage, only one should be implemented
endpoint :foo, :with_pagination
endpoint :foo, includes: :with_pagination
endpoint :foo do
with_pagination
end
endpoint :foo do
include_definition :with_pagination
end
# option B: just say that all endpoints have something
# B.1: beforehands
all_endpoints do
param :page
param :per_page
end
endpoint :foo # but how we exclude it?
# B.2: afterwards
endpoint :foo
all_endpoints.{select,reject,grep,whatever}.each do |endpoint|
endpoint.param :page
endpoint.param :per_page
end
What I like about either of options: you (almost) don't need to look at the TLAW docs to understand "what it does and why it does that" (unlike the "invented concept" global
proposal).
Option B.2 seems to be most straightforward in implementation (yes, doesn't work currently, but it is easy to fix), option A has an added value of "embedded declarations" (e.g. what has pagination and what not is extremely visible while being pretty compact) -- it is not 100% DRY, yet this "non-DRY-ness" close to the level of "repeating endpoint
keyword everywhere is not DRY" :)
So, WDYT?
I agree global
isn't a great name.
As for the concept, I still think there's something there. Maybe simply an option to add to param
, like specify_at: :end_point
(default would be :current
, but maybe some people would like :anywhere
that would allow it to be specified at any level between current and endpoint.
Exclusion, if need be, could be with an extra method exclude_param
.
Option A isn't DRY enough to my personal taste.
Option B2 feels less DSL-like to me.
Option B1 potential. A bit like before(:each)
of rspec
, we could have:
for_each :endpoint, except: %[foo bar] do
param :page
param :per_page
end
for_each :namespace, except: -> {|ns| ns.name !~ /_help$/ } do
yield # So endpoint below is listed at the end
endpoint :info do
desc 'Info about this'
end
end
I think for that option it would be best to have an exclusion list at the point of definition as above.
The nice thing about it is that it can deal with shared anything. OTOH, it introduces issues with ordering, so we could support yield
, and add it automatically at the end if it hasn't been called? But that's quite magical. Otherwise for_each :namespace, where: :after, except: ...
?
Alternatively, before_each :endpoint, except: ...
and after_each :namespace, ...
would probably be the clearest option.
As for the concept, I still think there's something there.
I am really not sure about this. When I designed the TLAW DSL, I tried to always follow what felt "natural for the domain" to me. While this "feeling of natural" is obviously an unclear thing, I can't remember some common term/approach used in lots of API docs to describe the concept of "param that most of endpoints have". As for pagination, it is typically either of two: described (non-DRY-ly) just for each endpoint, or having a separate section in API preface/afterword, just named "Pagination". I am not saying that the "proper, immediately understandable" term not exist... Yet I am still not sure what it can be.
OK, for the sake of moving forward, let's focus on option B (to be clear, I don't feel like option A non-DRY-ness is that critical, but as you are the driving force of the change, I prefer to achieve some compromise): some DSL to say "every endpoint have this".
There are several design decisions to make on the road. I had an unfortunate mix of them in the B.1 and B.2 options, now I want to unpack. Sorry for the long decision process—in fact, I still feel a responsibility for TLAWs DSL, and for features that important I am afraid of substantial implictions of the design decisions. So, the options we do have:
endpoint
/namespace
, declarative (context: current <Obj>Wrapper
-- hmm, why I haven't name them <Obj>Builder
, I now wonder?..)
b. more "imperative": context is not changed, but instead <Obj>Wrapper
is passed in the block, so you can do each { |ep| ep.param :page }
Now, one more very important thing: I believe that for all changes in DSL we should first consider a) minimal useful change and b) ...which could possibly be reused in other use-cases. Problem we are solving currently is, in fact, important yet small (just DRY-ing up some small things, that are already possible to do), and if this solution will require a whole new "section" of DSL; so my main target is to understand what useful "patterns" we can see here.
Examples of "big" things TLAW is currently missing is, say, approach to authorization with something more complex than static API keys; non-GET endpoints; support for information passing in headers (both request and response)—each of those makes library less useful than it could've been.
That's why for me something like this is still looking like "the best compromise": it is not 100% declarative, but somehow "that's the point"—it clearly looks like "not a new concept, but a small utility API to simplify what you already can do without it:
define do
...
namespace :foo do
endpoint :bar
endpoint :blah
endpoint :test
end
# Here we work with some special EndpointEnumerator, which is Enumerator's descendant
each_endpoint.grep_v(/test/).reject { |e| e.path.include?('bar') }.define do
# here we in context of EndpointWrapper
param :page
param :per_page
end
end
Oops, forgot about this while I was travelling...
1: easier implementation if the each
needs to be done beforehand. Seems pretty natural also I think
2: same as inside, definitely
3: best is probably to expose the name
within the wrapper (I don't think it is currently). This way nothing special to be done... One can do:
define do
after_each(:endpoint) do
break if name =~ /info$/
param :page
param :per_page
end
namespace :foo do
#...
end
end
Ping @zverok
OK, let me think about it till tomorrow, I am diving deeper in the codebase to try to cleanup things a bit.
OK, things got weird here. I started to clean up and modernize it, and eventually rewrote almost the whole codebase ¯\_(ツ)_/¯
On my way, I experimented with ways to solve the issue we are discussing, and chosen the approach that looks most reasonable to me for the vast majority of use cases -- the one with "shared definitions".
Sorry I wasted this amount of your time and energy to discuss that, and eventually got to solution you dislike :(
0.1.0.pre is out, for your library it would require this definition: https://gist.github.com/zverok/8123ea46536c2329c1dc5cd61117d086 I believe that it is the best trade-off between DRYness and obviousness: endpoints with no pagination just don't have it, and endpoints with one say this clearly yet concisely in their definition.
I'll be thankful if you could give a try to 0.1.0 in order I missed something huge on rewrite, but all of my tests and demos seem to work. Also, I tried to change your library to match all the changes in TLAW, and it was pretty easy to make it green, one other change besides gist above was changing doc.rb to this (much as you wanted it, I believe):
class << LibrariesIO::API
private def endpoint_doc(namespace, name)
id = if name == :info
namespace
else
[namespace, name.to_s.gsub('_', '-')].compact.join('-')
end
[LibrariesIO::API.docs_link, id].join('#')
end
def setup_all_doc(prefix = nil, base = self)
LibrariesIO::API.traverse(:endpoints) do |endpoint|
endpoint.docs_link = endpoint_doc(endpoint.parent.symbol, endpoint.symbol)
end
end
end
I have a working branch with 0.1.0.pre.
I'm not planning on using shared_def
/use_def
.
:-1: I'm not planning on using
shared_def
/use_def
.
¯\_(ツ)_/¯
Libraries.io's API has three parameters that can apply to every call. There
api_key
, which is quite natural to define as a top level parameter. There's alsopage
andper_page
, but those really only make sense at the endpoint level.I don't think there's a good way currently to handle that, right? I mean besides calling
param :page, ...
andparam :per_page, ...
for all endpoints obviously.I naively thought that declaring them at the top level would allow me to specify them from the endpoints (and possibly any intermediate namespace), but that's not the case.
Do you think that this is a need that
tlaw
should handle? Do you have an API that comes to mind?