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

feat: additional error properties #11

Merged
merged 25 commits into from
Nov 22, 2024
Merged

feat: additional error properties #11

merged 25 commits into from
Nov 22, 2024

Conversation

KSXGitHub
Copy link
Contributor

No description provided.

@KSXGitHub KSXGitHub marked this pull request as ready for review November 22, 2024 05:27
@KSXGitHub KSXGitHub marked this pull request as draft November 22, 2024 05:28
@zkochan zkochan requested a review from Copilot November 22, 2024 08:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 11 out of 17 changed files in this pull request and generated 1 suggestion.

Files not reviewed (6)
  • package.json: Language not supported
  • rust-toolchain: Language not supported
  • rustfmt.toml: Evaluated as low risk
  • .github/workflows/CI.yml: Evaluated as low risk
  • Cargo.toml: Evaluated as low risk
  • src/lib.rs: Evaluated as low risk

infinite_clone_test.mjs Outdated Show resolved Hide resolved
@KSXGitHub KSXGitHub marked this pull request as ready for review November 22, 2024 09:10
index.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old version of this file is moved to binding.js. The new index.js is a wrapper around binding.js.

binding.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was index.js.

@KSXGitHub
Copy link
Contributor Author

  • .github/workflows/CI.yml: Evaluated as low risk

  • Cargo.toml: Evaluated as low risk

  • src/lib.rs: Evaluated as low risk

Why did Copilot see them as low risk is beyond me.

Comment on lines 103 to -106
reflinkFileSync(
join(dir, 'file-does-not-exist.txt'),
join(dir, 'file1-copy.txt')
);
}).toThrow();
Copy link
Member

Choose a reason for hiding this comment

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

why not using await expect(...).rejects.toMatchObject({?

Copy link
Contributor Author

@KSXGitHub KSXGitHub Nov 22, 2024

Choose a reason for hiding this comment

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

I have replace all try catch block with expect().rejects (bcbbfaa).

@zkochan
Copy link
Member

zkochan commented Nov 22, 2024

I thought it was async. If it is sync, you can just use expect throws

@KSXGitHub
Copy link
Contributor Author

I thought it was async. If it is sync, you can just use expect throws

expect.toThrow doesn't provide toMatchObject.

@zkochan
Copy link
Member

zkochan commented Nov 22, 2024

ok, then use try/catch instead, as it was before

@KSXGitHub
Copy link
Contributor Author

Reverted.

@zkochan zkochan merged commit 313c935 into main Nov 22, 2024
50 checks passed
@KSXGitHub KSXGitHub deleted the error-codes branch November 22, 2024 12:30
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.

2 participants