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

refactor: switch to Rust #131

Merged
merged 12 commits into from
Apr 16, 2024
Merged

refactor: switch to Rust #131

merged 12 commits into from
Apr 16, 2024

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented May 11, 2022

Pending to fix tests and write tests for Rust's side of things

@Nytelife26
Copy link
Collaborator

Bindings tests need to be fixed to include Rust. They're failing because Cargo isn't present.

Copy link
Collaborator

@Nytelife26 Nytelife26 left a comment

Choose a reason for hiding this comment

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

No further comments for now. Good work :)
I'll check more in-depth later.

.gitignore Outdated Show resolved Hide resolved
npm/linux-x64-musl/package.json Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
src/games/tic_tac_toe.rs Outdated Show resolved Hide resolved
src/common/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Nytelife26 Nytelife26 left a comment

Choose a reason for hiding this comment

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

After this and the macro for tidying up, I'll move to performance analysis. If you'd like to do some of your own in the meantime at the machine code level, I'd recommend using Godbolt.

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/games/tic_tac_toe.rs Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

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

That'll be enough to go on with for now.

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
@Nytelife26
Copy link
Collaborator

Nytelife26 commented May 11, 2022

Since Player implements PartialEq, you can replace the redundant compare functions in each game module with this more idiomatic solution that can be stored in src/lib.rs.

This will also work for any other type that implements PartialEq, and is variadic.

macro_rules! many_eq {
    ($x:expr, $y:expr $(,$rest:expr)*) => {
        $x == $y && many_eq!($y $(,$rest)*)
    };
    ($x:expr) => { true };
}

@kyranet kyranet force-pushed the refactor/rusty branch 3 times, most recently from 728db83 to e328b6c Compare May 15, 2022 23:49
Copy link
Collaborator

@Nytelife26 Nytelife26 left a comment

Choose a reason for hiding this comment

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

I equally dislike myself for not mentioning this sooner. Although, fortunately, it's probably the easiest change I've pointed out.

index.d.ts Outdated Show resolved Hide resolved
@kyranet kyranet marked this pull request as ready for review May 16, 2022 17:51
@kyranet kyranet requested a review from favna May 16, 2022 17:52
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
src/games/connect_four.rs Outdated Show resolved Hide resolved
src/games/connect_four.rs Show resolved Hide resolved
src/games/tic_tac_toe.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Nytelife26 Nytelife26 left a comment

Choose a reason for hiding this comment

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

LGTM after that final nitpick.

Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

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

Barring this I have no idea about the production code but I trust Nyte reviewed that. I also have no idea how to version and publish it so that will be on your shoulders.

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
node-version: 16
check-latest: true
cache: yarn
architecture: ${{ matrix.settings.architecture }}
Copy link
Member

Choose a reason for hiding this comment

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

Resolved without reply

strategy:
fail-fast: false
matrix:
settings:
Copy link
Member

Choose a reason for hiding this comment

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

Resolved without resolution. This still applies.

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated
"build:debug": "napi build --platform --js index.cjs --pipe \"prettier -w\"",
"update": "yarn upgrade-interactive",
"prepublishOnly": "napi prepublish -t npm",
"prepare": "husky install .github/husky",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"prepare": "husky install .github/husky",
"postinstall": "husky install .github/husky",
"postpack": "pinst --enable"

package.json Outdated Show resolved Hide resolved
@Nytelife26
Copy link
Collaborator

Status on this?

@kyranet
Copy link
Member Author

kyranet commented Sep 8, 2022

There are TODOs left to do before merging, and I'm sadly too busy for that at the moment :/

@Nytelife26
Copy link
Collaborator

There are TODOs left to do before merging, and I'm sadly too busy for that at the moment :/

If there's anything you want me to do, let me know. I'm moving into student accommodation soon so I'll be mostly free until the 23rd.

@kyranet kyranet force-pushed the refactor/rusty branch 5 times, most recently from 3b23a2d to d633a06 Compare July 14, 2023 15:50
@kyranet kyranet force-pushed the refactor/rusty branch 3 times, most recently from 8ad2726 to 7ccb5c8 Compare July 21, 2023 14:58
@kyranet kyranet merged commit 7260e2c into main Apr 16, 2024
28 checks passed
@kyranet kyranet deleted the refactor/rusty branch April 16, 2024 09:09
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