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

Snapbox 0.3.1 Breaking Change in \ normalization #119

Closed
dbanty opened this issue Aug 5, 2022 · 6 comments · Fixed by #120
Closed

Snapbox 0.3.1 Breaking Change in \ normalization #119

dbanty opened this issue Aug 5, 2022 · 6 comments · Fixed by #120
Labels
breaking-change bug Not as expected

Comments

@dbanty
Copy link
Contributor

dbanty commented Aug 5, 2022

0.3.1 of Snapbox was a breaking change for me—as I'm using it to compare some generated TOML here. The \ in these cases are not path separators, so should not be normalized.

If this is intentional, I can just replace my usage of assert_eq_path with a custom Assert that disables normalization—but I wanted to report the issue just in case it was unintentional.

Thanks!

@epage
Copy link
Contributor

epage commented Aug 5, 2022

I assume that failure came from knope-dev/knope#218 which is upgrading from 0.2.10 to 0.3.1, so breaking changes are allowed.

However, you should have already been seeing that in 0.2. We normalize and text, including paths. I'm looking into this to better understand what is happening.

However, in 0.3.1 we introduced a setting to control this, normalize_paths. An example of it in use is at https://github.com/crate-ci/typos/blob/master/crates/varcon/tests/codegen.rs#L10

@epage
Copy link
Contributor

epage commented Aug 5, 2022

9ba8123 is the commit that caused it to start failing

@epage epage added the bug Not as expected label Aug 5, 2022
@epage
Copy link
Contributor

epage commented Aug 5, 2022

Looking at the commit, it looks like we mean to only normalize paths in the "matches" case and not the "equal" case but in changing the code up, one of those got swapped.

Now that we have normalize_paths, I wonder if we should normalize paths for the equal case and not just the matches case. That would involve updating the directory version to match what was done in 9ba8123

@epage
Copy link
Contributor

epage commented Aug 5, 2022

btw the work in that commit was one step in a series of refactors for #92 which would make it easier to diff toml, even if we still normalize.

@dbanty
Copy link
Contributor Author

dbanty commented Aug 5, 2022

I assume that failure came from knope-dev/knope#218 which is upgrading from 0.2.10 to 0.3.1, so breaking changes are allowed.

I thought the tests were passing against 0.3.0 but I never merged that update, so I could be mistaken.

So is the path forward to normalize in all the places? Just wondering if I should start updating my tests to explicitly disable normalization.

@dbanty
Copy link
Contributor Author

dbanty commented Aug 15, 2022

Looks like my tests are passing again under 0.3.2 🥳. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change bug Not as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants