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

chore: update deno_lint binary used in CI to v0.5.0 #10652

Merged
merged 10 commits into from
May 18, 2021

Conversation

magurotuna
Copy link
Member

This PR updates third_paty submodule so we can use the latest deno_lint in CI.
The latest deno_lint brings no-unused-vars and no-deprecated-deno-api, which spot some of the existing code as errors. I dealt with them by fixing the code or adding ignore directives.

Additionally, I removed ESLint directives because we don't use ESLint now and deno_lint hasn't supported them since #8226.

@magurotuna magurotuna force-pushed the update-third-party branch 3 times, most recently from 64bd8a5 to 4ee8447 Compare May 16, 2021 06:58
@magurotuna
Copy link
Member Author

Hmm, looks like this relative import doesn't work in CI... 🤔 I have no idea how to resolve it.

import { writeAllSync } from "../../test_util/std/io/util.ts";

Comment on lines 7 to 10
const bytesWritten = Deno.stderr.writeSync(data);
if (bytesWritten !== data.byteLength) {
throw new TypeError("failed to report test result");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we make sure that all data have been written to stderr like the following?

Suggested change
const bytesWritten = Deno.stderr.writeSync(data);
if (bytesWritten !== data.byteLength) {
throw new TypeError("failed to report test result");
}
let bytesWritten = 0;
while (bytesWritten < data.byteLength) {
bytesWritten += Deno.stderr.writeSync(data.subarray(byteWritten));
}

Copy link
Member

Choose a reason for hiding this comment

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

that works too :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

so either is okay? then I'm fine with the current implementation of yours.
I feel a little bit skeptical though, because Deno.writeSync calls op_write_sync and then std::io::Write::write is used under the hood, which requires us to do a loop until all data are written if we wish to imitate something like write_all.
https://doc.rust-lang.org/std/io/trait.Write.html#examples

Copy link
Member

Choose a reason for hiding this comment

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

I'm aware, I just think the messages are small enough to never fail. But if you want to add the loop, that is fine by me. It is a little more robust.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Indeed the tests run in CI succeeded, but I'd prefer a robust one so I'll add the loop :)

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks :-)

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.

3 participants