radarlabs / s2

Node.js JavaScript / TypeScript bindings for Google S2
Apache License 2.0
132 stars 19 forks source link

Typos in Readme #54

Closed spendres closed 1 year ago

spendres commented 2 years ago

1. line: const pointAtLevel14 = point.parent(s2Level); should be: const pointAtLevel14 = point.parent(s2level); // both lowercase l

2. line: console.log(coveringCells.contains(new s2.CellId(s2LatLong))); should be: console.log(coveringCells.has(new s2.CellId(s2LatLong)));

jkao commented 1 year ago

Want to open a PR?

spendres commented 1 year ago

I can. Do you have a branch or tag you prefer me to use?

On Thu, Dec 29, 2022 at 7:56 PM Jeff Kao @.***> wrote:

Want to open a PR?

— Reply to this email directly, view it on GitHub https://github.com/radarlabs/s2/issues/54#issuecomment-1367667342, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEVLYBQ2NEEBEN6P7DBA63WPYXKLANCNFSM6AAAAAARSJUY2U . You are receiving this because you authored the thread.Message ID: @.***>

jkao commented 1 year ago

We don't have a strict convention for branch names right now. As long as this issue is back linked in the PR, we should be good to go! :)

On Thu, Dec 29, 2022, 5:31 PM Steven Endres @.***> wrote:

I can. Do you have a branch or tag you prefer me to use?

On Thu, Dec 29, 2022 at 7:56 PM Jeff Kao @.***> wrote:

Want to open a PR?

— Reply to this email directly, view it on GitHub https://github.com/radarlabs/s2/issues/54#issuecomment-1367667342, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAEVLYBQ2NEEBEN6P7DBA63WPYXKLANCNFSM6AAAAAARSJUY2U

. You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/radarlabs/s2/issues/54#issuecomment-1367676087, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFUSG2IRC3KT7B7SMXRXWTWPY3PNANCNFSM6AAAAAARSJUY2U . You are receiving this because you commented.Message ID: @.***>

spendres commented 1 year ago

Okay, just used your current branch. Just committed the update for you to review.

jkao commented 1 year ago

it may actually require a fork and a PR against this repo for it to work. I can update it in my existing PR.

spendres commented 1 year ago

That will work. It’s a very small change.

On Fri, Dec 30, 2022 at 2:21 PM Jeff Kao @.***> wrote:

it may actually require a fork and a PR against this repo for it to work. I can update it in my existing PR.

— Reply to this email directly, view it on GitHub https://github.com/radarlabs/s2/issues/54#issuecomment-1368061314, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEVLYHFZGK5WTY3CV5PP63WP4Y5BANCNFSM6AAAAAARSJUY2U . You are receiving this because you authored the thread.Message ID: @.***>

jkao commented 1 year ago

Fixed: https://github.com/radarlabs/s2/commit/96c395af13f74d449e3ce7f8d1a58a8fde08e350