larsyencken / csvdiff

Generate a diff between two tabular datasets expressed in CSV files.
BSD 3-Clause "New" or "Revised" License
132 stars 31 forks source link

Ignore columns #16

Closed dandeezy closed 7 years ago

dandeezy commented 8 years ago

This branch adds ignore column functionally by adding any columns you want this app to ignore via command line --ignore as many times as you want. These columns won't show up in diff but will be shown in patches!

Lars, I can't seem to be able to fix the tests. I don't know why csvdiff_cmd returns exit code -1 . The application works fine and I've manually tested it. Can you help me fix these few failing tests?

dandeezy commented 8 years ago

Added '--ignore', 'extra' parameters to csvdiff_cmd runner in tests. Tests still fail.

larsyencken commented 8 years ago

Hi Dan, thanks for the patch! I'm on summer break and without computer for another week, I'll take a look once I'm back and see if I can work out why the tests still fail. Just seeing the exit code being different is a little frustrating, I agree. On Fri, 22 Jul 2016 at 02:41, Dan Murad notifications@github.com wrote:

Added '--ignore', 'extra' parameters to csvdiff_cmd runner in tests. Tests still fail.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/larsyencken/csvdiff/pull/16#issuecomment-234425064, or mute the thread https://github.com/notifications/unsubscribe-auth/AACMrL8L6rZInrKoMOV_AaeMuSeMA3UMks5qYBGjgaJpZM4JSWDT .

dandeezy commented 8 years ago

No problem Lars, I’ll tell my team to pip install my branch zip.

Enjoy your break!

Talk to you later.

From: Lars Yencken [mailto:notifications@github.com] Sent: Saturday, July 23, 2016 12:18 AM To: larsyencken/csvdiff csvdiff@noreply.github.com Cc: Dan Murad github@danamurad.com; Author author@noreply.github.com Subject: Re: [larsyencken/csvdiff] Ignore columns (#16)

Hi Dan, thanks for the patch! I'm on summer break and without computer for another week, I'll take a look once I'm back and see if I can work out why the tests still fail. Just seeing the exit code being different is a little frustrating, I agree. On Fri, 22 Jul 2016 at 02:41, Dan Murad <notifications@github.com mailto:notifications@github.com > wrote:

Added '--ignore', 'extra' parameters to csvdiff_cmd runner in tests. Tests still fail.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/larsyencken/csvdiff/pull/16#issuecomment-234425064, or mute the thread https://github.com/notifications/unsubscribe-auth/AACMrL8L6rZInrKoMOV_AaeMuSeMA3UMks5qYBGjgaJpZM4JSWDT .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/larsyencken/csvdiff/pull/16#issuecomment-234702176 , or mute the thread https://github.com/notifications/unsubscribe-auth/AMZ2OpTJLon62CwMlRlOYE68PJOElwDvks5qYbH6gaJpZM4JSWDT . https://github.com/notifications/beacon/AMZ2OofIoBF0_-gLuBcNTsACLq65vI_jks5qYbH6gaJpZM4JSWDT.gif

larsyencken commented 8 years ago

Ah, ok. Basically, the existing tests cover the loop of diffing a.csv and b.csv with csvdiff, generating a patch, then applying the patch to a.csv with csvpatch. It should be a closed loop that gives you the same result as b.csv.

In this case, you changed some of the test files, and needed to add the --ignore option to csvpatch too. I'd make new tests instead of changing the existing ones though.

In terms of the code change, a nicer way of doing this would be to use the ignored columns to create a view of the data that doesn't have those columns, when loading records. That way diffing or patching code doesn't need to change.

Got time to try and adjust your pull request, or would you like me to have a crack at this other approach?

dandeezy commented 8 years ago

Sure take a crack at it. That is a better way, I agree.

kevinlr5 commented 7 years ago

I would like to give this a go...

dandeezy commented 7 years ago

@kevinlr5 go for it

larsyencken commented 7 years ago

Yeah, things have been more busy for me than I realised. Please take a stab at it!

larsyencken commented 7 years ago

Just merged https://github.com/larsyencken/csvdiff/pull/19 which implemented this in the view style. Thanks for prompting for this new feature.