Aleph-Alpha / ts-rs

Generate TypeScript bindings from Rust types
MIT License
1.15k stars 115 forks source link

bug: `#[ts(export_to = "../path")]` can cause `diff_paths` to fail #247

Closed escritorio-gustavo closed 8 months ago

escritorio-gustavo commented 8 months ago

This PR reproduces the potential for paths containing .. to break diff_paths

NyxCode commented 8 months ago

I think we can dumb it down a bit, something like this.
That inlines clean_path and uses the fact that we know that the input to clean_path starts at the root to simplify it.

escritorio-gustavo commented 8 months ago

Oh, I have kinda been doing the opposite lol

I separated absolute and clean into their own modules and traits. I haven't pushed this commit yet, so I can totally just reverse it and replace with the gist you sent if you want.

I do like this version (with the changes in #248) though, because it's a more general solution, so if we ever need it again, it's just there

escritorio-gustavo commented 8 months ago

~Another thing I did is change current_dir to CARGO_MANIFEST_DIR as you suggested. They are always the same as far as ts-rs is concerned and CARGO_MANIFEST_DIR is already used elsewhere, so it would be inconsistent to use current_dir.~

Turns out there is a difference, as I mentioned in #248, the example directory exports to ts-rs/bindings when using CARGO_MANIFEST_DIR

~I also used the std::env! macro instead of std::env::var().unwrap()~ No longer relevant since I went back to current_dir

escritorio-gustavo commented 8 months ago

Hey, sorry I vanished, had to do some stuff at work, so it may take a few hours before I'm able to make more changes.

I do still have the commits I mentioned, but as I said it goes pretty much in the opposite direction you asked for. Do you want me to push it anyway so you can have a look?

escritorio-gustavo commented 8 months ago

Do you want me to push it anyway so you can have a look?

Hey @NyxCode, I did push it as a PR into this branch #248. I'm gonna have to leave again for a few hours. If you don't like it I will close #248 and apply the gist instead.

escritorio-gustavo commented 8 months ago

Hey @NyxCode, check out this version, it does inline and simplify the cleaning of the path like in the gist you submitted. It also renames the function to absolute and moves it into a mod called path, so if std::path::absolute ever comes to stable Rust, we can just add std:: to these calls

NyxCode commented 8 months ago

Hey! Sorry for not responding sooner, still busy atm. Will probably stay that way until march 28th.

This PR looks great! Doesn't overcomplicate anything while keeping export.rs tidy.
Only thing that caught my eye is that the test should probably have an assert!(Path::new("..").is_file()) to make sure everything is actually in the right place.
Besides that, this looks nice, thanks! Feel free to merge it.

escritorio-gustavo commented 8 months ago

Hey! Sorry for not responding sooner, still busy atm. Will probably stay that way until march 28th.

No problem, take your time!

Only thing that caught my eye is that the test should probably have an assert!(Path::new("..").is_file()) to make sure everything is actually in the right place.

I'm a bit confused, which test should have that assert? Also, doesn't Path::new("..").is_file() always evaluate to false due to treating .. as a directory?

NyxCode commented 8 months ago

Could have worded that better, sry about that 😆
From my side, we can merge this. Nice work.

escritorio-gustavo commented 8 months ago

Could have worded that better, sry about that 😆

Don't worry about it xD