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

CSV differ #57

Closed
nightscape opened this issue Nov 4, 2014 · 8 comments
Closed

CSV differ #57

nightscape opened this issue Nov 4, 2014 · 8 comments

Comments

@nightscape
Copy link

Hi Beth,

I've created a CSV differ for pact.
It doesn't have tests yet and uses cucumber tables for diffing (which seems a little awkward), but it works in manual tests on an adapted example.
Maybe you can have a look if this goes in the right direction.
I don't know how much time I can spend on this, but hopefully get to write some basic tests at least.

Best
Martin

@bethesque
Copy link
Member

That's fantastic! Great work. I think you're on the right track. Without the tests I have no idea what the output will look like though, so I hope you can find the time.

I agree, having a Cucumber dependency is a little awkward. Something I have discovered about gem dependencies is that copy-paste is not always an evil, and is often preferable to adding a whole gem dependency when all you want is a specific bit of functionality that is unlikely to ever change (with a credit at the top of course, to say where it has been copied from). Do you think it would be possible to extract just the Table class?

@nightscape
Copy link
Author

I'll have a look if the diffing can be extracted from the Table class, I hope it's not too coupled. First I have to create a use case for my company though to allow me to invest more time into pact.
One thing I'm not sure of how to do (according to my understanding of the code, it probably can't be done yet) is how to define per request if additional columns or rows are allowed. The differ itself has an options hash but at least the RSpec matcher doesn't pass any options. I assume that you could create a configured instance of a Differ in the configuration instead of the class, but this would only allow setting options for all requests with the same mime-type, right?

@bethesque
Copy link
Member

Yes, you can only set the Differ per mime-type, not per request. But this
may not be a problem - keep reading.

So, Pact follows Postel's law for matching. Requests should have only what
is expected, and no more. Responses should allow extra things in them that
can be ignored by the consumer (as there may be more than one consumer, and
they may be interested in different columns). This makes more obvious sense
for a JSON request/response than as CSV, I'll admit. If the response
columns have headers, and the client looks up the data by knowing the names
of the headers, I would say that extra columns should be allowed all the
time, and order shouldn't matter, because any extra columns/order would
just be ignored. If the response columns are keyed by index, then any extra
columns on the end should be ok, but not any in the middle, and order
should matter.

The difference between the request and the response diffing is that the
option allow_unexpected_keys: false is passed in to the Differ for
requests, and as you have noticed, no options are passed in to the Differ
options for responses (it defaults to allowing unexpected keys).

It seems to me that there should be two types of CSV Differs - one that
caters for a CSV with headers, and one that caters for a CSV with no
headers.

What do you think?

On Wed, Nov 5, 2014 at 9:06 PM, Martin Mauch notifications@github.com
wrote:

I'll have a look if the diffing can be extracted from the Table class, I
hope it's not too coupled. First I have to create a use case for my company
though to allow me to invest more time into pact.
One thing I'm not sure of how to do (according to my understanding of the
code, it probably can't be done yet) is how to define per request if
additional columns or rows are allowed. The differ itself has an options
hash but at least the RSpec matcher doesn't pass any options
https://github.com/realestate-com-au/pact/blob/master/lib/pact/provider/rspec/matchers.rb#L36.
I assume that you could create a configured instance of a Differ in the
configuration
https://github.com/crealytics/pact/blob/1757b54e163090b2e4084fc3929ef012998937d5/example/animal-service/spec/service_consumers/pact_helper.rb#L16
instead of the class, but this would only allow setting options for all
requests
with the same mime-type, right?


Reply to this email directly or view it on GitHub
https://github.com/realestate-com-au/pact/issues/57#issuecomment-61784855
.

@nightscape
Copy link
Author

Hi Beth,

sorry for the long delay after your detailed answer.
I got pulled into other stuff and I don't know when I'll be able to continue on CSV support for pact.
If you want, I'll close this issue for the time being and update/reopen it when I have time to continue with it.

Best
Martin

@bethesque
Copy link
Member

Okey dokey. If I find someone else who needs it, are you happy to publish the gem, or share gem publishing permissions?

@nightscape
Copy link
Author

I've published my initial version here:
http://rubygems.org/gems/pact-csv
I'm happy to share gem publishing permissions if someone wants to continue.
Shall I give you permissions just in case?

@bethesque
Copy link
Member

Just in case! I'd prefer not to write my actual rubygems email address on a github issue, can I send it to info at crealytics.com?

@nightscape
Copy link
Author

You can send it to mauch at crealytics de. Also it seems rubygems.org is already publishing my private email address :p

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

No branches or pull requests

2 participants