FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add `step` optarg to `r.range` by marshall007 · Pull Request #5545 · rethinkdb/rethinkdb · GitHub

Add step optarg to r.range - #5545

Open
marshall007 wants to merge 4 commits into
rethinkdb:mainfrom
marshall007:marshall_3148
Open

Add step optarg to r.range#5545
marshall007 wants to merge 4 commits into
rethinkdb:mainfrom
marshall007:marshall_3148

Conversation

Copy link
Copy Markdown
Contributor

Fixes #3148. Thus far I've only updated the argument parsing logic for r.range in the JS driver.

Copy link
Copy Markdown
Member

Thanks for the pull request @marshall007. I think this is your first C++ PR? :-)

I'll take a look at it later.

danielmewes added this to the 2.3-polish milestone Mar 19, 2016
danielmewes self-assigned this Mar 19, 2016
Comment thread src/rdb_protocol/datum_stream.cc Outdated
while (!is_exhausted()) {
double next = safe_to_double(start++);
double next = safe_to_double(start);
start+=step;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Per our usual style, there should be spaces around the operator: start += step;

Copy link
Copy Markdown
Member

I added a few comments, but looks good mostly.

Are you planning to add support in the other drivers as well? We can take over that part if you prefer.

Copy link
Copy Markdown
Member

Just saw that there was some discussion in #3148 on whether this option should be called stride or step. I personally like step better. I'll ask a few people around here for what they think on Monday.

Copy link
Copy Markdown
Contributor Author

I think this is your first C++ PR? :-)

@danielmewes yep! :D I've made updates based on your comments and refactored all those separate optarg checks in the JS driver into util.isPlainObject(obj). I replied to your comment regarding the behavior when step == 0. Just let me know if you think we should support that.

Are you planning to add support in the other drivers as well? We can take over that part if you prefer.

Would be great if someone else could take over, thanks!

Copy link
Copy Markdown
Member

Thanks for updating the PR and for the isPlainObject refactor.

Let's keep disallowing step == 0. It avoids potentially unexpected behavior and allowing something later is always easier than the other way around (with respect to backwards compatibility).

The only remaining question is the name of the option. I'll talk to some people to figure this out and let you know later today.

Copy link
Copy Markdown
Member

@marshall007 After talking to some people, we settled on step.

The server side of this looks good to me. I haven't reviewed the JavaScript driver changes yet.

Copy link
Copy Markdown
Contributor Author

@danielmewes sweet! I went ahead and updated the Python driver too.

danielmewes modified the milestones: 2.3-polish, subsequent Apr 7, 2016
danielmewes modified the milestones: 2.4, subsequent Apr 21, 2016

srh commented Dec 10, 2017
edited
Loading

Copy link
Copy Markdown
Contributor

What's left is to do the Ruby and Java drivers. Working on that now...

Edit: Actually, I don't think people would like more new stuff getting pushed into 2.4. Making this on milestone 2.5.

srh modified the milestones: 2.4, 2.5 Dec 10, 2017
srh changed the base branch from next to v2.4.x April 17, 2022 01:14

srh commented Apr 17, 2022

Copy link
Copy Markdown
Contributor

Pointing this at v2.4.x in preparation for making that branch the main branch of the repo. I intend to look at this post-2.4.2 release.

srh changed the base branch from v2.4.x to main December 15, 2023 23:25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

ReQL proposal: add an optarg step to r.range

3 participants


Back | FazBrowse Home | New Git URL