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

fix: wrong hint for deprecated Deno APIs #1222

Merged
merged 4 commits into from
Jan 12, 2024
Merged

fix: wrong hint for deprecated Deno APIs #1222

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e716f5f
fix: wrong hint for deprecated Deno APIs)
skanehira Jan 5, 2024
68b0003
chore: add unittest for deprecated api hint
skanehira Jan 5, 2024
fcf5429
reset CI
bartlomieju Jan 7, 2024
045ca70
chore: fix wrong message
skanehira Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 62 additions & 12 deletions src/rules/no_deprecated_deno_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ impl TryFrom<(&str, &str)> for DeprecatedApi {
enum Replacement {
NameAndUrl(&'static str, &'static str),
Name(&'static str),
NameAndUrls(Vec<(&'static str, &'static str)>),
}

impl DeprecatedApi {
Expand All @@ -124,6 +125,17 @@ impl DeprecatedApi {
Replacement::NameAndUrl(name, url) => {
format!("Use `{}` from {} instead", name, url)
}
Replacement::NameAndUrls(name_and_urls) => {
let mut hint = String::from("Use ");
for (i, (name, url)) in name_and_urls.into_iter().enumerate() {
if i != 0 {
hint.push_str(" and ");
}
hint.push_str(&format!("`{}` from {}", name, url));
}
hint.push_str(" instead");
hint
}
Comment on lines +128 to +138
Copy link
Member

Choose a reason for hiding this comment

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

Can we get a unit test for the method hint? It would be great to confirm what hints are generated for complicated cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test 68b0003

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

I'm a bit worried that putting all the information in one line might look overwhelming to users.

image

One idea that came up to my mind is to separate hints into multiple lines, where each line contains just one reference to alternative API. Something like this:

Use the following APIs instead
  - `WritableStream` from https://deno.land/api?s=WritableStream
  - `ReadableStream.from` from https://deno.land/api?s=ReadableStream#variable_ReadableStream
  - `ReadableStream.pipeTo` from https://deno.land/api?s=ReadableStream#method_pipeTo_4

But I'm not really sure if this is doable with the current mechanism of showing diagnostics that depends on miette crate.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@magurotuna
Yes, such diagnostic results are indeed easy to understand.
However, I am not familiar with miette, so I don't know if it can output such diagnostic results
Also, since this is a sample implementation, I wonder if I need to work that hard
How about drafting a separate issue from this PR and improving it?
I would like to limit the scope of this issue to fixing the wrong message for now.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, we can do further improvement in a follow-up PR. Let's land it as-is then 👍

}
}

Expand All @@ -145,24 +157,37 @@ impl DeprecatedApi {
}

fn get_replacement(&self) -> Replacement {
const DENO_API: &str = "https://deno.land/api";
const BUFFER_TS: &str = "https://deno.land/std/io/buffer.ts";
const STREAMS_TS: &str = "https://deno.land/std/streams/conversion.ts";
const DENO_COMMAND_API: &str = "https://deno.land/api?s=Deno.Command";
const BUFFER_TS: &str =
"https://developer.mozilla.org/en-US/docs/Web/API/Streams_API";
const STREAMS_REDABLE_TS: &str = "https://deno.land/api?s=ReadableStream";
const STREAMS_REDABLE_PIPE_TO_TS: &str =
"https://deno.land/api?s=ReadableStream#method_pipeTo_4";
const STREAMS_REDABLE_FROM_TS: &str =
"https://deno.land/api?s=ReadableStream#variable_ReadableStream";
const STREAMS_WRITEABLE_TS: &str = "https://deno.land/api?s=WritableStream";
const STREAMS_TO_ARRAY_BUFFER_TS: &str =
"https://deno.land/std/streams/to_array_buffer.ts?s=toArrayBuffer";

use DeprecatedApi::*;
use Replacement::*;
match *self {
Buffer => NameAndUrl("Buffer", BUFFER_TS),
Copy => NameAndUrl("copy", STREAMS_TS),
Buffer => Name(BUFFER_TS),
Copy => Name(STREAMS_REDABLE_PIPE_TO_TS),
CustomInspect => Name("Symbol.for(\"Deno.customInspect\")"),
Iter => NameAndUrl("iter", STREAMS_TS),
IterSync => NameAndUrl("iterSync", STREAMS_TS),
Iter => Name(STREAMS_REDABLE_TS),
IterSync => Name(STREAMS_REDABLE_TS),
File => Name("Deno.FsFile"),
ReadAll => NameAndUrl("readAll", STREAMS_TS),
ReadAllSync => NameAndUrl("readAllSync", STREAMS_TS),
Run => NameAndUrl("Deno.Command", DENO_API),
WriteAll => NameAndUrl("writeAll", STREAMS_TS),
WriteAllSync => NameAndUrl("writeAllSync", STREAMS_TS),
ReadAll | ReadAllSync => NameAndUrls(vec![
("ReadableStream", STREAMS_REDABLE_TS),
("toArrayBuffer()", STREAMS_TO_ARRAY_BUFFER_TS),
]),
Run => NameAndUrl("Deno.Command", DENO_COMMAND_API),
WriteAll | WriteAllSync => NameAndUrls(vec![
("WritableStream", STREAMS_WRITEABLE_TS),
("ReadableStream.from", STREAMS_REDABLE_FROM_TS),
("ReadableStream.pipeTo", STREAMS_REDABLE_PIPE_TO_TS),
]),
}
}
}
Expand Down Expand Up @@ -535,4 +560,29 @@ Deno.readAll(reader);
],
}
}

#[test]
fn expect_deprecated_api_hint() {
let tests = vec![
(
"Buffer",
"Use `https://developer.mozilla.org/en-US/docs/Web/API/Streams_API` instead",
),
("copy", "Use `https://deno.land/api?s=ReadableStream#method_pipeTo_4` instead"),
("customInspect", "Use `Symbol.for(\"Deno.customInspect\")` instead"),
("File", "Use `Deno.FsFile` instead"),
("iter", "Use `https://deno.land/api?s=ReadableStream` instead"),
("iterSync", "Use `https://deno.land/api?s=ReadableStream` instead"),
("readAll", "Use `ReadableStream` from https://deno.land/api?s=ReadableStream and `toArrayBuffer()` from https://deno.land/std/streams/to_array_buffer.ts?s=toArrayBuffer instead"),
("readAllSync", "Use `ReadableStream` from https://deno.land/api?s=ReadableStream and `toArrayBuffer()` from https://deno.land/std/streams/to_array_buffer.ts?s=toArrayBuffer instead"),
("run", "Use `Deno.Command` from https://deno.land/api?s=Deno.Command instead"),
("writeAll", "Use `WritableStream` from https://deno.land/api?s=WritableStream and `ReadableStream.from` from https://deno.land/api?s=ReadableStream#variable_ReadableStream and `ReadableStream.pipeTo` from https://deno.land/api?s=ReadableStream#method_pipeTo_4 instead"),
("writeAllSync", "Use `WritableStream` from https://deno.land/api?s=WritableStream and `ReadableStream.from` from https://deno.land/api?s=ReadableStream#variable_ReadableStream and `ReadableStream.pipeTo` from https://deno.land/api?s=ReadableStream#method_pipeTo_4 instead"),
];

for test in tests {
let hint = DeprecatedApi::try_from(("Deno", test.0)).unwrap().hint();
assert_eq!(hint, test.1);
}
}
}