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

ruspty init #1

Merged
merged 34 commits into from
Jan 31, 2024
Merged

ruspty init #1

merged 34 commits into from
Jan 31, 2024

Conversation

szymonkaliski
Copy link
Member

The files that are important are: src/lib.rs, index.test.ts, the rest was auto-generated by various tools.

lhchavez
lhchavez reviewed Jan 23, 2024
View reviewed changes
Copy link
Contributor

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

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

let's get some @cbrewster or @abshkbh eyes for 🦀 style. looks good to me otherwise!

README.md Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
index.test.ts Show resolved Hide resolved
cbrewster
cbrewster reviewed Jan 23, 2024
View reviewed changes
Copy link
Member

@cbrewster cbrewster left a comment

Choose a reason for hiding this comment

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

just a few idiomatic Rust nitpicks, don't feel like you have to take my suggestions 😄

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
jackyzha0
jackyzha0 reviewed Jan 23, 2024
View reviewed changes
index.d.ts Show resolved Hide resolved
jackyzha0
jackyzha0 approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

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

lgtm, rust broadly looks good though i will defer most of that review to luis and connor :)

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@abshkbh abshkbh self-requested a review January 24, 2024 22:58
lhchavez
lhchavez approved these changes Jan 30, 2024
View reviewed changes
Copy link
Contributor

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

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

🦀 gud

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
abshkbh
abshkbh reviewed Jan 30, 2024
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
abshkbh
abshkbh approved these changes Jan 30, 2024
View reviewed changes
Copy link

@abshkbh abshkbh left a comment

Choose a reason for hiding this comment

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

LGTM % nit

@szymonkaliski szymonkaliski merged commit 3eeaee4 into main Jan 31, 2024
3 checks passed
@szymonkaliski szymonkaliski deleted the sk/init branch January 31, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackyzha0 jackyzha0 jackyzha0 approved these changes

@cbrewster cbrewster cbrewster left review comments

@abshkbh abshkbh abshkbh approved these changes

@lhchavez lhchavez lhchavez 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.
5 participants
@szymonkaliski @lhchavez @abshkbh @cbrewster @jackyzha0