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 assumes that file contents are the truth #227

Closed
not-my-profile opened this issue Aug 18, 2023 · 3 comments
Closed

snapbox assumes that file contents are the truth #227

not-my-profile opened this issue Aug 18, 2023 · 3 comments
Labels
A-snapbox Area: snapbox package

Comments

@not-my-profile
Copy link

assert_eq_path really just sounds like it checks a file content and a string for equality. But it's doc comment actually says:

Check if a value matches the content of a file

I think it's quite problematic that this assumption of the file content being the truth isn't reflected in the function name. Doc comments are quite easy to miss. Also snapbox does not provide an alternative method to assert that the file content matches the correct in-memory value, so in practice it will just be misused (often even most likely without noticing). Case in point tests/verify.rs in typos: add a duplicate word to assets/words.csv there and run cargo test -p typos-dict verify, which will print:

---- expected: assets/words.csv
++++ actual:   In-memory

This is wrong. What's expected in this case is in-memory and the file content is what has actually been found.

The discrepancy between function behavior and name should certainly be resolved. The issue could be addressed by having two functions with clear names, having one function where either parameter could be a path or an in-memory value, or just taking the easy way out and only reporting left and right.

@epage
Copy link
Contributor

epage commented Aug 18, 2023

So it sounds like this issue combines

  • Wanting to clarify in docs and API that the file is the source of truth
  • A new API that assumes the in-memory value is the source of truth

For the clarifying part, as I mentioned in #225, I expect #221 to change up the API in way that will make this clearer.

For the new API, that seems like a reasonable request for us to explore additions to the asserts we have for checking the filesystem. As this mostly focuses on the clarification and its best to have atomic Issues, please open a new issue for this.

@epage
Copy link
Contributor

epage commented Aug 18, 2023

For the clarifying, I'm closing assuming #221 will resolve this

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Aug 18, 2023
@epage epage added the A-snapbox Area: snapbox package label Aug 18, 2023
@not-my-profile
Copy link
Author

as I mentioned in #225, I expect #221 to change up the API in way that will make this clearer.

Could you add a comment to #221 elaborating how a new API could resolve these issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-snapbox Area: snapbox package
Projects
None yet
Development

No branches or pull requests

2 participants