Skip to content

Option to not to normalize backslash #91

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

Open
MiSawa opened this issue Apr 5, 2022 · 8 comments
Open

Option to not to normalize backslash #91

MiSawa opened this issue Apr 5, 2022 · 8 comments
Labels
A-trycmd Area: trycmd package enhancement Improve the expected

Comments

@MiSawa
Copy link

MiSawa commented Apr 5, 2022

I noticed that this fails due to "backslash to slash normalization".

bin.name = "bin-fixture"
stdout = '\'
[env.add]
stdout = '\'

It is probably useful for things that outputs paths, but is cumbersome for other things, e.g. a tool that outputs JSON, as test has to be stdout = '"/"escaped/""' for example instead of stdout = '"\"escaped\""', and there's no way to check if the output was actually correctly escaped with \.
I know that there's a bin mode, but I still want line separators normalized. It'd be great to have a way to turn off the path separator normalization feature.

@epage epage added the enhancement Improve the expected label Apr 5, 2022
@epage
Copy link
Contributor

epage commented Apr 21, 2022

The main question with this is deciding how to expose it.

@MiSawa
Copy link
Author

MiSawa commented Apr 22, 2022

It'd be great if it is exposed via TestCases for global default configuration like

let t = trycmd::TestCases::new()
t.output_normalize_path(false); // default: true (for compatibility)
t.output_normalize_line_separator(true); // default: true

and maybe per-test override via toml configuration

output_normalize_path = false
output_normalize_line_separator = true

though I guess it'll be a bit cumbersome to handle conflict with binary = true/false...

@epage
Copy link
Contributor

epage commented Apr 22, 2022

Is there a reason you'd want to offer control over line normalization instead of just associating that with binary?

though I guess it'll be a bit cumbersome to handle conflict with binary = true/false...

I'd just ignore or error if binary = true

btw I've been forgetting to write this up but #93 might also be useful

@MiSawa
Copy link
Author

MiSawa commented Apr 22, 2022

Is there a reason you'd want to offer control over line normalization instead of just associating that with binary?

Since I still want line separators to be normalized...

I'd just ignore or error if binary = true

Yeah that sounds reasonable.

btw I've been forgetting to write this up but #93 might also be useful

That'd be a great feature. But in my case, I want to verify not just as a json but some other things, like if it is properly indented and if the order of object entries are kept.

@epage
Copy link
Contributor

epage commented Apr 22, 2022

Since I still want line separators to be normalized...

More so I was wondering is if binary = false should automatically mean normalize line endings. If we had flags for each individual part of binary = false, then the flag might start to lose its value so I was wanting to explore how the all the pieces fit together.

Granted, I don't remember if we strip BOMs but that might be the most basic binary = false behavior.

That'd be a great feature. But in my case, I want to verify not just as a json but some other things, like if it is properly indented and if the order of object entries are kept.

Yes, I want to keep this in mind and give people control over that. Could you post that to the issue both so its explicitly recorded and so you'll be subscribed so you can make sure the solution for opting out is acceptable?

@MiSawa
Copy link
Author

MiSawa commented Apr 23, 2022

More so I was wondering is if binary = false should automatically mean normalize line endings. If we had flags for each individual part of binary = false, then the flag might start to lose its value so I was wanting to explore how the all the pieces fit together.

Ah sorry, I see what you meant now. Making binary = false normalize line separator works for my use case at least, and probably that's something others would expect.

Yes, I want to keep this in mind and give people control over that. Could you post that to the issue both so its explicitly recorded and so you'll be subscribed so you can make sure the solution for opting out is acceptable?

Filed #94 and #95 :)

@kraktus
Copy link
Contributor

kraktus commented May 3, 2022

Another solution would be to compare ouputs before normalization, and only normalize if the ouputs do not already matches (optimally even only normalise if it detects the failure is due to missing normalisation, but haven’t checked source code see if possible)

@epage
Copy link
Contributor

epage commented May 3, 2022

iirc I had considered that at one point but the challenge is it doesn't scale to different normalization options. Originally when I considered this, I was also wondering about the different replacement patterns supported, etc.

epage added a commit to epage/snapbox that referenced this issue Aug 3, 2022
This handles the snapbox side of assert-rs#91.  We'll need to update the plumbing
in trycmd to get this to all the right places.  For now this also lets
us experiment with it where we can more easily change the API.
epage added a commit to epage/snapbox that referenced this issue Aug 3, 2022
This handles the snapbox side of assert-rs#91.  We'll need to update the plumbing
in trycmd to get this to all the right places.  For now this also lets
us experiment with it where we can more easily change the API.

We still normalize paths when inserting substitions.  The two APIs
aren't connected at the moment to allow fixing this.  This will need
more consideration.
@settings settings bot removed A-snapbox labels Aug 14, 2023
@epage epage added the A-snapbox Area: snapbox package label Aug 15, 2023
@epage epage added A-trycmd Area: trycmd package and removed A-snapbox Area: snapbox package labels Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trycmd Area: trycmd package enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

3 participants