Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dynamically generating queue functions from Jedis' source code #18
Dynamically generating queue functions from Jedis' source code #18
Changes from 4 commits
1bfd4d8
47ff1dc
bec5f04
810939a
5716287
bfeed35
9825da0
63e6815
3e17915
030d0a9
721351d
04b11ad
5311106
d5f6982
066bb9d
188fd35
07eaf6e
376e73c
c696ff2
cfd6b2c
d206efa
ed9e222
519f203
88cb8c7
977a72e
206dd1e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to use jar in deps, you can use clj src from jedis, so you don't need to compile (generate the jar) to generate the build because it will take it from source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@J0sueTM I like to use the standard names used by the library we are wrapping, but the idea is for
clj-rq
to be simple and inclusive.Even if someone doesn't know the names of Redis functions but knows about queues, they should be able to use
clj-rq
. In other words, it's important to keeppush!
andpop!
calling their respective functions fromjedis
. These names are explicit about what is expected in execution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avelino Using macros puts a couple of limitations on ourselves. Of course what you're asking is feasible, but I don't think that parsing directions, and adding them into the function definition is within our time budget.
ourselves
It would involve changing the way we load the parameters from a given class, which could dismantle our macro. And for goods sake, writing that shivers my whole spine. But it's feasible nevertheless.
I'll scratch out a way to have the direction parsed when loading the parameters, as I'm currently implementing the static documentation, as layed out here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be that you are complicating more than necessary, how would I do it?
I would keep the same logic of
(defn push! [...])
, but changing the calls we make directly tojedis
to the functions built with the macro.does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avelino not just about the directions, in some functions we have things like a position (alike before and after) and some extra opts to add. If we straight forward make it "static" on this way, it would be easier to test out, since the tests we need to write involves some data transformation to fit the Redis patterns. But how can we improve it to maintain as dynamic as possible, @J0sueTM ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find out we can call specific opts while on code like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avelino @Felipe-gsilva There are too many variables to keep updating dynamically. I still think that we can of course check the direction from l or r, and check the method parameters and parse them appropiately. It just will take a lot more time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@J0sueTM I don't know if I understood the last comment - I imagine it's about @Felipe-gsilva's comment
It's clear my proposal to keep
push!
andpop!
as they were before and have them use the macro implementation, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avelino Yes. My comment was regarding Felipe's coment. This conversation all happened before our meeting, when I finally understood what you meant with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth writing by sharing what we will do in a separate comment so it doesn't get lost here
#18 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avelino give a look at the current implementation. Does it match with what you had in mind?