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.

Sign up for GitHub

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

Jump to bottom

feat: add bitfield command #1159

Merged
merged 6 commits into from
Jul 3, 2024
Merged

feat: add bitfield command #1159

merged 6 commits into from
Jul 3, 2024

Conversation

lewxdev
Copy link
Contributor

@lewxdev lewxdev commented Jun 28, 2024

resolves #1150

@lewxdev
Copy link
Contributor Author

lewxdev commented Jun 28, 2024 •
edited
Loading

I tried to model the code after the python implementation.

I can't get the tests to run. Could use an extra pair of eyes on it
There was a mistake on my part, the created tests run and pass

ogzhanolguncu
ogzhanolguncu reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor

@ogzhanolguncu ogzhanolguncu left a comment

Choose a reason for hiding this comment

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

Overall it looks great. There are just some minor convention issues.

pkg/commands/bitfield.ts Outdated Show resolved Hide resolved
pkg/commands/bitfield.ts Outdated Show resolved Hide resolved
pkg/redis.ts Show resolved Hide resolved
@lewxdev
feat: add bitfield to pipeline
9b2d5fa 
(also addresses pr comments)
@lewxdev
Copy link
Contributor Author

lewxdev commented Jul 1, 2024 •
edited
Loading

In order to add the bitfield command to the pipeline, I had to split BitFieldCommand into variants for use with redis and pipeline. This way, exec can return the result or a chainable pipeline respectively. I figured out how to do this without the need for two classes.

One thing that can be addressed is being able to infer the length of the result number[] based on the commands called (similar to how pipeline works) but the current type may be fine for now. I tried my hand at getting this to work, but I couldn't get a clean solution

@lewxdev
fix: code complexity
69ffc5b 
opt to pass private properties / methods from pipeline directly into the
constructor for the bitfield command. no need to create two separate
classes for command and pipeline
ogzhanolguncu
ogzhanolguncu approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@ogzhanolguncu ogzhanolguncu left a comment

Choose a reason for hiding this comment

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

Looking awesome. Thanks.

@ogzhanolguncu ogzhanolguncu merged commit c00b02d into upstash:main Jul 3, 2024
@lewxdev lewxdev deleted the add-bitfield branch July 4, 2024 01:21
renovate bot referenced this pull request in inabagumi/shinju-date Jul 4, 2024
@renovate
build(deps): update dependency @upstash/redis to ^1.32.0 (#4406)
b28d537 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@upstash/redis](https://togithub.com/upstash/upstash-redis) |
[`^1.31.6` ->
`^1.32.0`](https://renovatebot.com/diffs/npm/@upstash%2fredis/1.31.6/1.32.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@upstash%2fredis/1.32.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@upstash%2fredis/1.32.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@upstash%2fredis/1.31.6/1.32.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@upstash%2fredis/1.31.6/1.32.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>upstash/upstash-redis (@&#8203;upstash/redis)</summary>

###
[`v1.32.0`](https://togithub.com/upstash/redis-js/releases/tag/v1.32.0)

[Compare
Source](https://togithub.com/upstash/upstash-redis/compare/v1.31.6...v1.32.0)

#### What's Changed

- DX-1023: Add regex for base url by
[@&#8203;CahidArda](https://togithub.com/CahidArda) in
[https://github.com/upstash/redis-js/pull/1157](https://togithub.com/upstash/redis-js/pull/1157)
- feat: add bitfield command by
[@&#8203;lewxdev](https://togithub.com/lewxdev) in
[https://github.com/upstash/redis-js/pull/1159](https://togithub.com/upstash/redis-js/pull/1159)
- Resolve Auto Pipeline Arrappend Issue by
[@&#8203;CahidArda](https://togithub.com/CahidArda) in
[https://github.com/upstash/redis-js/pull/1165](https://togithub.com/upstash/redis-js/pull/1165)

#### New Contributors

- [@&#8203;lewxdev](https://togithub.com/lewxdev) made their first
contribution in
[https://github.com/upstash/redis-js/pull/1159](https://togithub.com/upstash/redis-js/pull/1159)

**Full Changelog**:
upstash/redis-js@v1.31.6...v1.32.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/inabagumi/shinju-date).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjEuOSIsInVwZGF0ZWRJblZlciI6IjM3LjQyMS45IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogzhanolguncu ogzhanolguncu ogzhanolguncu approved these changes

@CahidArda CahidArda CahidArda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing BITFIELD command
3 participants
@lewxdev @ogzhanolguncu @CahidArda