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.

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

move overloads of execute to @libsql/core/api (because its part of the public API) && add CONTRIBUTING.md #261

Merged
merged 11 commits into from
Sep 25, 2024

Conversation

DaBigBlob
Copy link
Contributor

move overloads of execute to @libsql/core/api (because its part of the public API)
and
add CONTRIBUTING.md to provide instructions to setup repo for experimentation and contribution

@DaBigBlob
Copy link
Contributor Author

some auto linting scripts (present in this repo) seem to have linted the migrate method right before git push

CONTRIBUTING.md Outdated Show resolved Hide resolved
@giovannibenussi
Copy link
Contributor

Thanks for working on these changes! They look good to me so I’m going to allow GitHub actions to run!
I can see that some changes are just a style change, so I’m not sure if you need to run prettier manually or not.

Anyways, looks good to me!

@giovannibenussi
Copy link
Contributor

Just read your comment sorry. We setup the repository so it runs prettier before pushing your changes so all of our code is formatted following the same rules. Seems that not all files are formatted though as commented in #256 so it would be nice if you can run prettier --write . (I think the whole command is npm run prettier --write .)

@DaBigBlob
Copy link
Contributor Author

Just read your comment sorry. We setup the repository so it runs prettier before pushing your changes so all of our code is formatted following the same rules. Seems that not all files are formatted though as commented in #256 so it would be nice if you can run prettier --write . (I think the whole command is npm run prettier --write .)

i originally didnt run this because this might've changed many files and that would've looked suspicious

@giovannibenussi
Copy link
Contributor

No problem, thanks for running it! Otherwise we may have issues with the rest of the active pull requests.
@penberg these changes looks good to me!

CONTRIBUTING.md Outdated
Comment on lines 3 to 4
- have `npm` installed and in path
- have `git` installed and in path
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- have `npm` installed and in path
- have `git` installed and in path
- have `npm` installed and in PATH
- have `git` installed and in PATH

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, I think the convention is $PATH no?

Copy link
Contributor

@alcpereira alcpereira Sep 17, 2024

Choose a reason for hiding this comment

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

AFAIK $PATH is only valid for Unix system. Windows GUI says Path but in cmd you can do echo %PATH%.
Thus my suggestion to just use PATH everywhere.

It's a nit pick anyway so feel free to ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me then!

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that this is a good opportunity to capitalize the instructions and leave a single space after each dash instead of four.

Suggested change
- have `npm` installed and in path
- have `git` installed and in path
- Have `npm` installed and in PATH
- Have `git` installed and in PATH

CONTRIBUTING.md Outdated
- have `npm` installed and in path
- have `git` installed and in path

# Setting up the repo for contributing
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the \*nix is relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dont use windows (or haiku or whatever) so i have no knowledge of non *nix
"... for *nix" does not mean not for windows. it simply means that this works for *nix. and does not specify anything for windows, etc

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I don't think that the *nix is relevant. I don't use windows either but a quick search and from my experience just mentioning the cd is enough.

Below is a rewrite of your instructions with some suggestions to improve clarity. Feel free to copy it completely or update what you think is relevant. I wanted to make sure that each step has a clear description and associated action.

Suggested change
# Setting up the repo for contributing
# Setting up the repo for contributing
- Clone this repository: `git clone https://github.com/tursodatabase/libsql-client-ts`
- Change the current working directory to the cloned repository: `cd libsql-client-ts`
- Install dependencies: `npm i`
- Change the current working directory to `libsql-core`'s workspace: `cd packages/libsql-core`
- Built the core package: `npm run build`
- Go back to the root directory to start making changes: `cd ../..`

CONTRIBUTING.md Outdated
Comment on lines 3 to 4
- have `npm` installed and in path
- have `git` installed and in path
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me then!

CONTRIBUTING.md Outdated
- have `npm` installed and in path
- have `git` installed and in path

# Setting up the repo for contributing
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I don't think that the *nix is relevant. I don't use windows either but a quick search and from my experience just mentioning the cd is enough.

Below is a rewrite of your instructions with some suggestions to improve clarity. Feel free to copy it completely or update what you think is relevant. I wanted to make sure that each step has a clear description and associated action.

Suggested change
# Setting up the repo for contributing
# Setting up the repo for contributing
- Clone this repository: `git clone https://github.com/tursodatabase/libsql-client-ts`
- Change the current working directory to the cloned repository: `cd libsql-client-ts`
- Install dependencies: `npm i`
- Change the current working directory to `libsql-core`'s workspace: `cd packages/libsql-core`
- Built the core package: `npm run build`
- Go back to the root directory to start making changes: `cd ../..`

CONTRIBUTING.md Outdated
Comment on lines 3 to 4
- have `npm` installed and in path
- have `git` installed and in path
Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that this is a good opportunity to capitalize the instructions and leave a single space after each dash instead of four.

Suggested change
- have `npm` installed and in path
- have `git` installed and in path
- Have `npm` installed and in PATH
- Have `git` installed and in PATH

@penberg penberg merged commit d84a4a4 into tursodatabase:main Sep 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants