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

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

Merged
merged 8 commits into from
Mar 6, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

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

@escritorio-gustavo escritorio-gustavo changed the title Reproduce path problem bug: #[ts(export_to = "../path")] can cause diff_paths to fail Feb 29, 2024
ts-rs/src/export.rs Outdated Show resolved Hide resolved
ts-rs/src/export.rs Outdated Show resolved Hide resolved
ts-rs/src/export.rs Outdated Show resolved Hide resolved
@NyxCode
Copy link
Collaborator

NyxCode commented Mar 1, 2024

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

This comment was marked as outdated.

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Mar 1, 2024

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

This comment was marked as off-topic.

@escritorio-gustavo

This comment was marked as outdated.

@escritorio-gustavo
Copy link
Contributor Author

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

@escritorio-gustavo escritorio-gustavo added bug Something isn't working P-HIGH This will be treated with high priority labels Mar 5, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Mar 6, 2024

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
Copy link
Contributor Author

escritorio-gustavo commented Mar 6, 2024

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
Copy link
Collaborator

NyxCode commented Mar 6, 2024

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

@escritorio-gustavo
Copy link
Contributor Author

Could have worded that better, sry about that 😆

Don't worry about it xD

@escritorio-gustavo escritorio-gustavo merged commit ffac0e2 into main Mar 6, 2024
10 checks passed
@escritorio-gustavo escritorio-gustavo deleted the repro-path-bug branch March 6, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P-HIGH This will be treated with high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants