feat: Catch typos in Cargo.toml and warn about them #446
Conversation
drager changed the title 
src/manifest/mod.rs Outdated
| let manifest: CargoManifest = serde_ignored::deserialize(manifest, |path| { | ||
| let path_string = path.to_string(); | ||
| | ||
| if path_string.contains("metadata") { |
There was a problem hiding this comment.
This may want to be metadata.wasm-pack perhaps? That way we won't accidentally warn about other metadata sections accidentally
There was a problem hiding this comment.
Oh right! That makes sense. Thanks!
There was a problem hiding this comment.
@alexcrichton: Hmm, if I do metadata.wasm-pack we wouldn't warn about misspelled wasm-pack. i.e. if someone writes [package.metadata.wasmpack.profile.dev.wasm-bindgen].
There was a problem hiding this comment.
That's true, but I personally would think that we can't land this while it warns about [package.metadata.some-unrelated-tool].
We can possibly do a lehvenstein distance thing (typically used for "did you typo this?") to figure out if it looks like wasm-pack specifically, but other crates/tools using [package.metadata] should not be warned about when using wasm-pack
There was a problem hiding this comment.
Sounds reasonable! Pushed some code for this.
src/manifest/mod.rs Outdated
| | ||
| unused_keys.iter().for_each(|path| { | ||
| PBAR.warn(&format!( | ||
| "\"{}\" is misspelled and will be ignored. Please check your Cargo.toml.", |
There was a problem hiding this comment.
This may want to be reworded a bit because we don't actually know if it's misspelled, perhaps this could just say that it's an ignored/unknown key?
There was a problem hiding this comment.
Yes, I was somewhat unsure what to put there and "unknown and will be ignored" sounds good.
Cargo.toml Outdated
| name = "wasm-pack" | ||
| readme = "README.md" | ||
| repository = "https://github.com/ashleygwilliams/wasm-pack.git" | ||
| version = "0.5.1" |
There was a problem hiding this comment.
It looks like some unrelated changes snuck in here?
There was a problem hiding this comment.
Yeah, I used cargo add to add strsim and it did some reordering. I will push a commit that restores it.
Cargo.toml Outdated
| | ||
| [dependencies.openssl] | ||
| optional = true | ||
| version = "0.10.11" |
There was a problem hiding this comment.
I think the movement here may be an unrelated change?
There was a problem hiding this comment.
Yeah, I used cargo add to add strsim and it did some reordering. I will push a commit that restores it.
src/manifest/mod.rs Outdated
| let manifest: CargoManifest = serde_ignored::deserialize(manifest, |path| { | ||
| let path_string = path.to_string(); | ||
| | ||
| if levenshtein(WASM_PACK_METADATA_KEY, &path_string) == levenshtein_threshold { |
There was a problem hiding this comment.
Could this perhaps have some more guards like testing that the key starts with package.metadata and then just running levenshtein over the key afterwards, ideally only matching "wasm-pack" up against the next part of the key?
Also, I think this may perhaps want to be <= threshold to ensure that exact matches are warned about
There was a problem hiding this comment.
Sounds good! Ah, ofc it should be <=.
src/manifest/mod.rs Outdated
| let path_string = path.to_string(); | ||
| | ||
| if levenshtein(WASM_PACK_METADATA_KEY, &path_string) == levenshtein_threshold { | ||
| has_possible_typo = true; |
There was a problem hiding this comment.
I think it's still good to warn about all the keys here, so it's fine for this to take the same path as the one below where it just throws a new key in the set to get warned about, that way the exact key that wasn't used is printed as well and the user doesn't have to guess
There was a problem hiding this comment.
Yes, that makes sense. Much better to print the key so the user doesn't have to guess.
| Code-wise looks great to me, thanks! |
| Awesome! Thank you for the guidance and feedback! |
fitzgen added the TWiRaWA 4 participants
This intends to fix #443.