Refactor and test info field styling #1214
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This moves the bulk of the logic for writing styled info into the `InfoField` trait as a provided set of methods. This makes each type that implements `InfoField` more testable.
spenserblack
force-pushed
the
snapshot-tests
branch
from
a44a629 to
25d17d9
Compare
spenserblack
changed the title
o2sh
approved these changes
Nov 15, 2023
There was a problem hiding this comment.
You're absolutely right; it makes more sense to have the formatting logic of info fields inside info_field trait 😅. Additionally, get_style and format_number could also be moved out of info/mod.rs into info/utils/mod.rs.
Thanks as well for implementing the snapshot tests; as you mentioned, they are better suited for visual outputs. Only downside being the multiple snapshots folders.
We could wait and see where mitsuhiko/insta#416 goes 🙂 |
|
This is minor IMO, no need to block the PR. We'll reconsider as we add more snapshot tests and await the upstream issue resolution. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is to illustrate where we might want to switch to snapshot tests. With the change to alignment in #1207, for example, a snapshot test makes it much easier for a review to confirm proper alignment. "assert contains" isn't really an ideal test -- who knows how many leading or trailing characters there are from the test alone?
I'm leaving this as a draft right now because, as you can see, these snapshots look kind of weird. That's because they're testing.value(), which assumes that it will be preceded by a title. Thus, theTODOcomment.Edit: After working on this PR a bit to get more useful snapshot tests, I realized there was an opportunity to refactor a bit. This PR now moves the logic that styles info fields onto the
InfoFieldtrait, and then uses styled snapshot tests (you can view the styled output withcat path/to/snapshot) to test and visualize the outputs.