Add: Redis Geospatial Command Stages #328
Conversation
|
Warning Rate limit exceeded@UdeshyaDhungana has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughA new "Geospatial Commands" extension was added to the Redis course definition, introducing support for a suite of geospatial commands. Eight new stages were defined, each with a dedicated markdown description file detailing the implementation requirements and test procedures for commands like GEOADD, GEOPOS, GEODIST, and GEOSEARCH. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant RedisImplementation
Tester->>RedisImplementation: GEOADD key lon lat name
RedisImplementation-->>Tester: RESP Integer (count of elements added)
Tester->>RedisImplementation: GEOPOS key name1 [name2 ...]
RedisImplementation-->>Tester: RESP Array (coordinates or nils)
Tester->>RedisImplementation: GEODIST key name1 name2 [unit]
RedisImplementation-->>Tester: RESP Bulk String (distance or nil)
Tester->>RedisImplementation: GEOSEARCH key FROMLONLAT lon lat BYRADIUS radius unit
RedisImplementation-->>Tester: RESP Array (matching names)
Tester->>RedisImplementation: GEOSEARCH key FROMLONLAT lon lat BYBOX width height unit
RedisImplementation-->>Tester: RESP Array (matching names)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -0,0 +1,34 @@ | |||
| In this stage, you'll add support for inserting multiple locations in one `GEOADD` command. | |||
There was a problem hiding this comment.
Let's ignore this from scope and stick to adding one location at once, similar to what we did with the SUBSCRIBE command. It also makes logs simpler when we show one place per line everywhere.
We can keep the test for retrieving multiple locations though, the response type there is an array so it'll be weird for users to hardcode an array with a single element when implementing. The syntax is also shorter than this (GEOPOS key loc_1 loc_2)
|
|
||
| ``` | ||
|
|
||
| In the response, both latitude and longitude values are rounded to 6 digits after the decimal point. |
There was a problem hiding this comment.
Let's not be this strict - just assert that the message starts with invalid. Also less for us to explain re: rounding.
|
|
||
| It returns the distance as a string, encoded as a RESP Bulk String. The precision of the response is up to 4 digits after the decimal. | ||
|
|
||
| Calculating the distance when two latitude and longitude pairs are given is not as straightforward as using Pythagorean theorem. Since the earth is a sphere, we have to account for its curvature as well. The [Haversine's Formula](https://en.wikipedia.org/wiki/Haversine_formula#Example) is used to calculate distance in such cases. See how redis implements it [here](https://github.com/redis/redis/blob/4322cebc1764d433b3fce3b3a108252648bf59e7/src/geohash_helper.c#L228C1-L228C72). |
There was a problem hiding this comment.
Let's shorten, no need to mention pythagorean theorem etc.:
Redis uses the [Haversine's Formula](link) to calculate the distance between two points. You can see how this is done in the Redis source code [here](link).
| The `GEODIST` command returns the distance between two members. It can also take a optional last parameter which is the unit in which to express the distance in. The default unit is meters. The valid units are meters(m), kilometers(km), miles(mi), or feet(ft). | ||
| The syntax for `GEODIST` command is: | ||
| ``` | ||
| GEODIST <key> <location1> <location2> [unit] |
There was a problem hiding this comment.
Let's not mention unit here and just cover the default unit - we can mention units in the next stage explanation
| ``` | ||
|
|
||
| ### Notes | ||
| - For greatest accuracy, it is recommended to calculate the distance in feet, since it is the unit with smallest precision, and convert that into other units. |
There was a problem hiding this comment.
Would this really matter? Is this what Redis does?
There was a problem hiding this comment.
Redis does this using meters, and converts it to other units
Ref: https://github.com/redis/redis/blob/d86cf6610144249f846f3358ea8cc1cf6409b3e8/src/geo.c#L134
I will remove this.
There was a problem hiding this comment.
Ah, yeah I think it's okay to not mention - doesn't seem like it'd make a big difference in accuracy
course-definition.yml
Outdated
| # Geospatial Commands | ||
| - slug: "zt4" | ||
| primary_extension_slug: "geospatial" | ||
| name: "Add a location" |
There was a problem hiding this comment.
Great work on the names, this is perfect! Only note is that "Validate Coordinates" doesn't need capitalization - just do "Validate coordinates".
- Rename stage desription files to match the latest format
|
|
Going to take an extra day or two to review this - want to see if I can hook up the automated review stuff here. |
course-definition.yml
Outdated
| - slug: "ek6" | ||
| primary_extension_slug: "geospatial" | ||
| name: "Measure Distance" |
course-definition.yml
Outdated
| - slug: "ra4" | ||
| primary_extension_slug: "geospatial" | ||
| name: "Unit Conversion" |
| In this stage, you'll add support for adding a single location under a key using the `GEOADD` command. | ||
|
|
||
| ### The `GEOADD` command | ||
| The `GEOADD` command adds one or more location (with longitude, latitude, and name) to a key. If the key doesn’t exist, it is created. If the key exists, the locations are appended under that key. |
There was a problem hiding this comment.
| The `GEOADD` command adds one or more location (with longitude, latitude, and name) to a key. If the key doesn’t exist, it is created. If the key exists, the locations are appended under that key. | |
| The `GEOADD` command adds a location (with longitude, latitude, and name) to a key. If the key doesn’t exist, it is created. If the key exists, the location is appended to that key. |
We don't have to mention the one or more case since we don't use it anywhere
| ./your_program.sh | ||
| ``` | ||
|
|
||
| It will then send a `GEOADD` command specifying a key, random values of latitude and longitude, and also a random name for that co-ordinate. |
There was a problem hiding this comment.
Let's include the actual command here, many users only look at the Tests section and skip the explanation sections
| In this stage, you'll add support for validating the latitude and longitude provided in the `GEOADD` command. | ||
|
|
||
| ### The `GEOADD` command (Validate coordinates) | ||
| The latitude and longitudes used in the GEOADD command should be in a certain range. Valid longitudes are from -180 to 180 degrees. Valid latitudes are from -85.05112878 to 85.05112878 degrees. Both of these limits are inclusive. |
There was a problem hiding this comment.
| The latitude and longitudes used in the GEOADD command should be in a certain range. Valid longitudes are from -180 to 180 degrees. Valid latitudes are from -85.05112878 to 85.05112878 degrees. Both of these limits are inclusive. | |
| Latitudes and longitudes used in the GEOADD command should be in a certain range as per [ESPG:3857](https://epsg.io/3857). Valid longitudes are from -180 to 180 degrees. Valid latitudes are from -85.05112878 to 85.05112878 degrees. Both of these limits are inclusive. |
|
|
||
| ``` | ||
| $ redis-cli | ||
| > GEOADD location_key 200 100 foo |
There was a problem hiding this comment.
Let's keep the command and response in the same code block to be consistent, also use redis-cli GEOADD ... so it's simpler to read
| ``` | ||
|
|
||
| ### Notes | ||
| - In case of invalid co-ordinates, the tester is lenient in checking error messages so you don't have to stick to the exact format Redis uses. The exact format it checks for is `invalid` (case-insensitive). Examples of error message strings that will pass the tests: |
There was a problem hiding this comment.
| - In case of invalid co-ordinates, the tester is lenient in checking error messages so you don't have to stick to the exact format Redis uses. The exact format it checks for is `invalid` (case-insensitive). Examples of error message strings that will pass the tests: | |
| - The tester is lenient in checking error messages so you don't have to stick to the exact format Redis uses. The exact format it checks for is `invalid` (case-insensitive). Examples of error message strings that will pass the tests: |
(We're already in the context of invalid co-ordinates for this stage)
There was a problem hiding this comment.
Can we check for a bit more than just "invalid"? Maybe check for presence of latitude / longitude too, depending on which is invalid?
There was a problem hiding this comment.
I am not sure if I understood correctly. Assuming that we ought to check for which of the values (latitude/longitude) is incorrect and checking for the indication in the error response, Redis does not specify which value latitude/longitude is invalid.
> GEOADD p 15 240 foo
(error) ERR invalid longitude,latitude pair 15.000000,240.000000
> GEOADD p 240 23 foo
(error) ERR invalid longitude,latitude pair 240.000000,23.000000
So, I am not sure if we should test this.
We can instead add another test case. For e.g. with a non-numeric values:
127.0.0.1:6379> GEOADD p 15 foo bar
(error) ERR value is not a valid float
There was a problem hiding this comment.
Ah yep, I know Redis doesn't specify which one is valid/invalid - but a user might want to implement a more "specific" error mesage like "ERR invalid longitude xyz".
Basically we want to be slightly lenient here but not so lenient as to allow just "invalid" to go through. I thought one way to do this was to also assert that "longitude" or "latitude" are present depending on which was an invalid value. Would that work?
We don't need the non-numeric one, seems like an edge case. We tend to avoid these in general unless they help with teaching a concept / forcing an implementation. In this case, the invalid longitude / latitude is useful to teach users about the mercator projection usage.
| - `Invalid longitude,latitude` | ||
| - `invalid` | ||
|
|
||
| - If a `GEOADD` command specifies multiple locations, and if the co-ordinates of any one of those locations is invalid, it returns the error message and all the locations are discarded. |
There was a problem hiding this comment.
We don't cover adding multiple locations so we can remove this
There was a problem hiding this comment.
Added one round, @UdeshyaDhungana can you revise based on these comments (ensure other stages adhere to them too), and then re-tag for review please?
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
course-definition.yml (2)
689-692: Back-tick placement is broken in UNSUBSCRIBE blurbmarketing_md: In this stage, you'll add support for the `UNSUBSCRIBE command`, which …The back-tick is opened before UNSUBSCRIBE but closed after command, so Markdown renders it incorrectly.
-In this stage, you'll add support for the `UNSUBSCRIBE command`, which is used… +In this stage, you'll add support for the `UNSUBSCRIBE` command, which is used…
720-724: Typo: “comamnd” → “command”-… using the `GEODIST` comamnd. +… using the `GEODIST` command.stage_descriptions/geospatial-01-zt4.md (1)
7-31: Add language identifiers to fenced blocksMarkdown-lint flags every fenced block because no language is specified. Add a language to silence CI and enable syntax highlighting:
-``` -GEOADD <key> <longitude> <latitude> <name> -``` +```text +GEOADD <key> <longitude> <latitude> <name> +``` … -``` -> GEOADD places 13.361389 38.115556 "Palermo" -(integer) 1 -``` +```redis +> GEOADD places 13.361389 38.115556 "Palermo" +(integer) 1 +``` … -``` -./your_program.sh -``` +```bash +./your_program.sh +```Apply the same pattern to the
$ redis-cli GEOADD …snippet below (bashorredisboth fine).stage_descriptions/geospatial-04-ia8.md (1)
6-48: Specify fence languages & drop leading$where output isn’t shownAll code blocks are unlabeled (MD040) and some use
$even though no output follows (MD014). Example fixes:-``` -./your_program.sh -``` +```bash +./your_program.sh +``` -``` -$ redis-cli +```redis +redis-cli > GEOADD …Repeat for the RESP-encoding block (
text) and the GEOPOS example (redis).stage_descriptions/geospatial-06-ra4.md (1)
5-41: Add languages to fenced blocks for lint complianceEach
…needs a language tag; suggested values:
- Syntax line →
text- Interactive examples →
redis- Shell invocation →
bash- RESP encoding →
textApply consistently across the file to clear markdown-lint MD040.
stage_descriptions/geospatial-07-rm9.md (1)
9-54: Label code fences & keep sample output blocks distinctTo satisfy MD040 & improve readability:
-``` -GEOSEARCH key FROMLONLAT longitude latitude BYRADIUS distance [km|ft|m|mi] -``` +```text +GEOSEARCH key FROMLONLAT <lon> <lat> BYRADIUS <dist> [km|ft|m|mi] +``` Use `redis` for interactive queries, `bash` for `./your_program.sh`, and `text` for RESP arrays. Also, ensure the final “RESP array” example is fenced independently so highlighting doesn’t bleed into prose. </blockquote></details> <details> <summary>stage_descriptions/geospatial-08-hv8.md (1)</summary><blockquote> `6-8`: **Add language identifiers to all fenced code blocks.** `markdownlint` flags every anonymous block. Use `redis`, `bash`, or `text` as appropriate to improve syntax-highlighting and satisfy MD040. ```diff -``` +```redis GEOSEARCH key FROMLONLAT longitude latitude BYBOX width height [km|ft|m|mi] -``` +``` …apply the same change to the blocks starting at lines 12, 27, 45 and 66 – picking `redis` for CLI sessions and `text` for raw RESP bytes.Also applies to: 12-23, 27-33, 45-53, 66-80
stage_descriptions/geospatial-05-ek6.md (1)
6-8: Add missing fenced-block languages.Same MD040 issue as other files. Use:
redisfor CLI interactionsbashfor shell invocations (./your_program.sh)textfor raw RESPApplying this across all six blocks will silence the linter and aid readability.
Also applies to: 15-15, 22-24, 28-33, 40-45
stage_descriptions/geospatial-02-ck3.md (1)
7-15: Specify language for each code block & fix list indentation.
- Add a language tag (
redis,bash, ortext) to every fenced block to satisfy MD040.- The unordered list under “Notes” (lines 50-53) is indented 4 spaces instead of 2, triggering MD007.
- Prefer a consistent unordered-list marker (
-everywhere) to avoid MD004.- - `ERR invalid longitude,latitude pair 200,100` - - `ERR Invalid longitude,latitude` - - `ERR invalid` + - `ERR invalid longitude,latitude pair 200,100` + - `ERR Invalid longitude,latitude` + - `ERR invalid`Also applies to: 19-21, 26-31, 35-37, 41-46
stage_descriptions/geospatial-03-xg4.md (1)
7-9: Resolve markdownlint violations: add languages, normalise list style.• Tag every fenced block (
redis,bash,text).
• The nested list under “Notes” mixes*with default-; use one style consistently.
• Indent list items by 2 spaces to pass MD007.Example fix for the first code block:
-``` +```redis GEOPOS <key> <location1> <location2> … -``` +```Apply a similar change to the remaining blocks and replace
*with-.Also applies to: 12-22, 34-36, 40-46, 50-55, 57-64, 70-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
course-definition.yml(2 hunks)stage_descriptions/geospatial-01-zt4.md(1 hunks)stage_descriptions/geospatial-02-ck3.md(1 hunks)stage_descriptions/geospatial-03-xg4.md(1 hunks)stage_descriptions/geospatial-04-ia8.md(1 hunks)stage_descriptions/geospatial-05-ek6.md(1 hunks)stage_descriptions/geospatial-06-ra4.md(1 hunks)stage_descriptions/geospatial-07-rm9.md(1 hunks)stage_descriptions/geospatial-08-hv8.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
stage_descriptions/geospatial-02-ck3.md
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nates values, it will expect an error. For example, the tester might send your pro...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
stage_descriptions/geospatial-02-ck3.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
stage_descriptions/geospatial-01-zt4.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
30-30: Dollar signs used before commands without showing output
(MD014, commands-show-output)
stage_descriptions/geospatial-07-rm9.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
stage_descriptions/geospatial-06-ra4.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
stage_descriptions/geospatial-04-ia8.md
6-6: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
stage_descriptions/geospatial-05-ek6.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
stage_descriptions/geospatial-03-xg4.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
72-72: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
73-73: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
74-74: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
75-75: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
76-76: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
stage_descriptions/geospatial-08-hv8.md
6-6: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 YAMLlint (1.37.1)
course-definition.yml
[error] 130-130: trailing spaces
(trailing-spaces)
[error] 741-741: no new line character at the end of file
(new-line-at-end-of-file)
[error] 741-741: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test_course_definition / test (swift)
- GitHub Check: test_course_definition / test (scala)
- GitHub Check: test_course_definition / test (ocaml)
- GitHub Check: test_course_definition / test (kotlin)
- GitHub Check: test_course_definition / test (haskell)
- GitHub Check: test_course_definition / test (clojure)
🔇 Additional comments (1)
course-definition.yml (1)
741-741: Missing newline at EOFMany tools (git, POSIX text utilities, some YAML parsers) expect a newline-terminated file. Add one here.
| ``` | ||
| > GEODIST places Catania Rome | ||
| "537215.1152" | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Distance values are inconsistent for the same city pair.
Lines 12-14 show Catania ↔ Rome as 537 215.1152 m while lines 36-38 expect 166.2742 (unit not stated).
Readers (and test-writers) will be confused and may implement wrong logic.
Action:
- Decide on the authoritative example (meters vs. kilometers) and keep a single value.
- Clarify the unit in the second snippet if you intend to demonstrate a later stage with unit conversion.
Also applies to: 35-38
🤖 Prompt for AI Agents
In stage_descriptions/geospatial-05-ek6.md around lines 11 to 14 and 35 to 38,
the distance values for the city pair Catania and Rome are inconsistent, showing
537215.1152 meters in one place and 166.2742 (unit unclear) in another. Decide
whether to use meters or kilometers as the standard unit for distance throughout
the document, update the values accordingly to be consistent, and explicitly
state the unit of measurement in all examples to avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
stage_descriptions/geospatial-05-ek6.md (2)
5-8: Add fenced-block languages to silence markdown-lintSix code blocks trigger MD040 errors. Add a language identifier (
redis,bash,text, etc.) right after the opening ```.Example fix:
-``` +```redis GEODIST <key> <location1> <location2>Apply similar changes to the other five blocks.
Also applies to: 11-14, 22-25, 27-31, 35-38, 42-45
50-60: Mixed list marker styleLines 52-58 use
*while the rest of the doc uses-. Pick one list-marker style for consistency (MD004).course-definition.yml (2)
722-723: Spelling typo in marketing text
"GEODIST comamnd"→"GEODIST command"- marketing_md: In this stage, you'll add support for calculating the distance between two locations using the `GEODIST` comamnd. + marketing_md: In this stage, you'll add support for calculating the distance between two locations using the `GEODIST` command.
691-691: Back-tick wraps the wrong textOnly the Redis keyword should be in back-ticks.
- marketing_md: In this stage, you'll add support for the `UNSUBSCRIBE command`, which is used to unsubscribe from a channel. + marketing_md: In this stage, you'll add support for the `UNSUBSCRIBE` command, which is used to unsubscribe from a channel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
course-definition.yml(2 hunks)stage_descriptions/geospatial-05-ek6.md(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
course-definition.yml
[error] 130-130: trailing spaces
(trailing-spaces)
[error] 740-740: no new line character at the end of file
(new-line-at-end-of-file)
🪛 markdownlint-cli2 (0.17.2)
stage_descriptions/geospatial-05-ek6.md
6-6: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
52-52: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
54-54: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
55-55: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
56-56: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
57-57: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
58-58: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
60-60: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test_course_definition / test (swift)
- GitHub Check: test_course_definition / test (odin)
- GitHub Check: test_course_definition / test (zig)
- GitHub Check: test_course_definition / test (ocaml)
- GitHub Check: test_course_definition / test (kotlin)
- GitHub Check: test_course_definition / test (rust)
- GitHub Check: test_course_definition / test (scala)
- GitHub Check: test_course_definition / test (java)
- GitHub Check: test_course_definition / test (go)
- GitHub Check: test_course_definition / test (haskell)
- GitHub Check: test_course_definition / test (cpp)
- GitHub Check: test_course_definition / test (c)
- GitHub Check: test_course_definition / test (clojure)
- GitHub Check: Cursor Bugbot
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
course-definition.yml (2)
130-130: Trailing whitespace still present – YAML lint will fail
This was flagged earlier and is unchanged. Remove trailing spaces to keep linters green.- +
738-741: Missing EOF newline + trailing spaces break YAML validity
Line 741 has trailing spaces and there’s no terminating newline, which most YAML parsers require.- marketing_md: In this stage, you'll add support for searching locations near a coordinate within a rectangular bounding box.␠␠ + marketing_md: In this stage, you'll add support for searching locations near a coordinate within a rectangular bounding box. +
🧹 Nitpick comments (4)
course-definition.yml (2)
689-692: Unmatched back-tick around command name
The opening back-tick beforeUNSUBSCRIBEis never closed, rendering the remainder of the line as code in most renderers.- marketing_md: In this stage, you'll add support for the `UNSUBSCRIBE command`, which is used to unsubscribe from a channel. + marketing_md: In this stage, you'll add support for the `UNSUBSCRIBE` command, which is used to unsubscribe from a channel.
720-724: Typo: “comamnd” → “command”
Spelling mistakes leak into rendered docs & dashboards.- marketing_md: In this stage, you'll add support for calculating the distance between two locations using the `GEODIST` comamnd. + marketing_md: In this stage, you'll add support for calculating the distance between two locations using the `GEODIST` command.stage_descriptions/geospatial-03-xg4.md (1)
7-9: Add language identifiers to fenced code blocks for proper rendering & linting
markdownlintMD040 is raised multiple times. Prefix the blocks withredis(orbash/text) to satisfy linters and enable syntax-highlighting.-``` +```redis(Apply to all GEOPOS/GEOADD/RESP examples in the file.)
Also applies to: 11-23, 41-46, 50-64 </blockquote></details> <details> <summary>stage_descriptions/geospatial-02-ck3.md (1)</summary><blockquote> `7-15`: **Consistently tag code blocks & fix list indentation** MD040 and MD007 warnings stem from missing language labels and uneven list levels. Quick wins: 1. Add `redis` after each opening ``` 2. Re-indent nested bullet lists with 2-space increments. Example: ```diff -``` +# redisand
- - Start with the phrase "ERR invalid" (case-insensitive) - - Contain the word latitude if the latitude value is invalid + - Start with the phrase "ERR invalid" (case-insensitive) + - Contain the word **latitude** if the latitude value is invalidAlso applies to: 19-23, 26-37, 41-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
course-definition.yml(2 hunks)stage_descriptions/geospatial-02-ck3.md(1 hunks)stage_descriptions/geospatial-03-xg4.md(1 hunks)stage_descriptions/geospatial-05-ek6.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- stage_descriptions/geospatial-05-ek6.md
🧰 Additional context used
🪛 YAMLlint (1.37.1)
course-definition.yml
[error] 130-130: trailing spaces
(trailing-spaces)
[error] 741-741: no new line character at the end of file
(new-line-at-end-of-file)
[error] 741-741: trailing spaces
(trailing-spaces)
🪛 LanguageTool
stage_descriptions/geospatial-02-ck3.md
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nates values, it will expect an error. For example, the tester might send your pro...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
stage_descriptions/geospatial-02-ck3.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
44-44: Inconsistent indentation for list items at the same level
Expected: 8; Actual: 9
(MD005, list-indent)
44-44: Unordered list indentation
Expected: 4; Actual: 9
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
stage_descriptions/geospatial-03-xg4.md
6-6: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
52-52: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
54-54: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
55-55: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
56-56: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
57-57: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
58-58: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
60-60: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test_course_definition / test (haskell)
- GitHub Check: test_course_definition / test (scala)
- GitHub Check: test_course_definition / test (c)
- GitHub Check: test_course_definition / test (zig)
- GitHub Check: test_course_definition / test (rust)
- GitHub Check: test_course_definition / test (swift)
- GitHub Check: test_course_definition / test (cpp)
- GitHub Check: test_course_definition / test (ocaml)
- GitHub Check: test_course_definition / test (go)
- GitHub Check: test_course_definition / test (odin)
- GitHub Check: test_course_definition / test (java)
- GitHub Check: test_course_definition / test (kotlin)
- GitHub Check: test_course_definition / test (clojure)
|
Will look at this first thing tomorrow morning & make the final edits myself! |
- Re-wrote instructions in detail including the encode/decode algorithm
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
course-definition.yml
Outdated
| - slug: "ek6" | ||
| primary_extension_slug: "geospatial" | ||
| name: "Measure distance" |
There was a problem hiding this comment.
| name: "Measure distance" | |
| name: "Calculate distance" |
(Consistent with other stages)
course-definition.yml
Outdated
| name: "Search within radius" | ||
| difficulty: hard | ||
| marketing_md: In this stage, you'll add support for searching locations near a coordinate within a given radius. |
There was a problem hiding this comment.
Let's mention the command we're using here
| $ ./your_program.sh | ||
| ``` | ||
|
|
||
| It will then send a `GEOADD` command specifying a key, laitude, longitude, and location name. |
There was a problem hiding this comment.
| It will then send a `GEOADD` command specifying a key, laitude, longitude, and location name. | |
| It will then send a `GEOADD` command specifying a key, latitude, longitude, and location name. |
course-definition.yml
Outdated
| primary_extension_slug: "geospatial" | ||
| name: "Calculate location score" | ||
| difficulty: easy |
There was a problem hiding this comment.
Let's adjust these difficulties based on your best guess @UdeshyaDhungana - don't think this one would be easy for example
| ### The `GEOADD` command | ||
| The `GEOADD` command adds a location (with longitude, latitude, and name) to a key. It stores the location as a [sorted set](https://redis.io/docs/latest/develop/data-types/sorted-sets/) under the specified key. If the key doesn’t exist, a new sorted set is created under the specified name and the location is inserted to it. If the key exists, the location is inserted in the sorted set. | ||
|
|
||
| The syntax for `GEOADD` command is |
There was a problem hiding this comment.
Let's not use the syntax view here - show the example usage first (i.e. the "show don't tell" technique) and then if needed, explain what the arguments are further. For most simple commands users will be able to infer what the values are if we just use appropriate names
| LONGITUDE_MAX = 180 | ||
| ``` | ||
|
|
||
| You can see how the official Redis implementation does this [here](https://github.com/redis/redis/blob/ff2f0b092c24d5cc590ff1eb596fc0865e0fb721/src/geohash.c#L141) |
There was a problem hiding this comment.
| You can see how the official Redis implementation does this [here](https://github.com/redis/redis/blob/ff2f0b092c24d5cc590ff1eb596fc0865e0fb721/src/geohash.c#L141) | |
| You can see how the official Redis implementation does this [here](https://github.com/redis/redis/blob/ff2f0b092c24d5cc590ff1eb596fc0865e0fb721/src/geohash.c#L141). |
(Sentence usage)
|
|
||
| 2. The bits of latitude and longitude are interleaved to form a 64 bit unsigned integer. | ||
|
|
||
| Let's consider the bits of latitude are `X31 X30 X29 ... X2 X1 X0` (32 bit integer), and the bits of longitude are `Y31 Y30 Y29 ... Y2 Y1 Y0` (32 bit integer). The end goal here is to construct a 64 bit integer whose digits will look like this: |
There was a problem hiding this comment.
@UdeshyaDhungana is there a reason we're using the terminology X31 X30 X29 .. instead of X0 X1 X2...?
There was a problem hiding this comment.
@rohitpaulk I 'correlated' LSBs with lower numbers and MSBs with higher numbers. And since the bit pattern is laid out in the same format (MSBs...LSBs) in the explanation below, I found it intuitive to write it down like this.
Let me know if I should change this.
There was a problem hiding this comment.
Ah, okay I think since we're mostly focusing on order & interleaving here it'd be simpler to just use the 0..31 ordering. In fact, let's go one step and do 1..32.
Reasoning: we should minimize the mental math a user needs to do when looking at this paragraph vs the code example below where you show the interleaved version
There was a problem hiding this comment.
Sure! I'll change it.
| return lat | (lon << 1) | ||
| ``` | ||
|
|
||
| Let's focus only on the latitude value for now and call it x. This is a 32-bit integer made up of bits like X31, X30, ..., X0. The end goal here is to spread these bits out so that each one is separated by a zero. In other words, the goal here is to turn x into a 64-bit value: 0 X31 0 X30 0 X29 ... 0 X1 0 X0. |
There was a problem hiding this comment.
Let's move this explanation into comments on the code block - explain just-in-time, avoid confusing users first and then explaining later
| The `GEOPOS` command returns the longitude and latitude of the specified location. Its syntax is | ||
|
|
||
| ``` | ||
| GEOPOS <key> <location1> |
There was a problem hiding this comment.
Same note as usual - avoid syntax, just show the example first. Add syntax / explanation only if it seems necessary after
There was a problem hiding this comment.
Hmm, I don't see this xg4 - hb5 split in the course-definition - @UdeshyaDhungana can you confirm what's up here?
There was a problem hiding this comment.
Sorry, I forgot to update the stages in the course definition. I'll update it to reflect the actual stages.
Summary by CodeRabbit
New Features
Documentation