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

diff preview #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

diff preview #82

wants to merge 1 commit into from

Conversation

garmoshka-mo
Copy link

 Approvals::ApprovalError:
   Approval Error: Received file does not match approved:
   spec/fixtures/approvals/ampcontroller/renders_article.received.html
   spec/fixtures/approvals/ampcontroller/renders_article.approved.html
   approved fragment: uper-bowl-opening-night-2016-best-moments
   received fragment: uper-bowl-ening-night-2016-best-moments-a

Gives instant idea of what could be wrong.
Plus indispensable, when approval error happens on external isolated environment like Travis or Circle-ci.

@geeksam
Copy link
Contributor

geeksam commented May 28, 2021

@garmoshka-mo Hi! I've volunteered to help out with maintaining this gem. It's going to take me a while to get up to speed enough to decide what to do with this PR, but I can at least start out by asking: is this still something you want to see in the approvals gem?

If so, I might look for a way we can make this pluggable, because various kinds of data (XML, HTML, JSON, CSV, plain text, etc) might each benefit from their own way of presenting diffs.

For example, I see that the CLI takes a -d flag that allows you to specify a diff command to be used, but the implementation is currently locked up in the CLI implementation, so that feature isn't really available if you're using approvals from RSpec. If I can refactor the code to make "diff presentation" a first-class concept in the rest of the gem, that might benefit a wider variety of use cases...

@garmoshka-mo
Copy link
Author

Hi @geeksam
it was pretty long time ago, I don't work anymore on project where I used this library

@geeksam
Copy link
Contributor

geeksam commented Jun 4, 2021

@garmoshka-mo thanks! Thought that might be the case. I'll leave this open as a reminder to think about diffing at some point, though. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants