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

Easier-to-parse diff #221

Closed
lutzky opened this issue Jan 10, 2022 · 11 comments · Fixed by #239
Closed

Easier-to-parse diff #221

lutzky opened this issue Jan 10, 2022 · 11 comments · Fixed by #239
Labels
kind/feature New feature or request lifecycle/keep-alive Denotes an issues or PR that should never be considered stale.

Comments

@lutzky
Copy link
Contributor

lutzky commented Jan 10, 2022

Currently gmailctl diff can sometimes show output like this:

- to: {long@gmail.com list@gmail.com of@gmail.com aliases@gmail.com that@gmail.com apply@gmail.com to@gmail.com this@gmail.com rule@gmail.com}
+ to: {long@gmail.com list@gmail.com of@gmail.com aliases@gmail.com that@gmail.com apply@gmail.com this@gmail.com rule@gmali.com}

...it's not fun figuring out what's missing there.

The cmp package does what I mean, I think: https://go.dev/play/p/E5RwmDYIsvX -

  main.WithSlices{
  	To: []string{
  		... // 4 identical elements
  		"that@gmail.com",
  		"apply@gmail.com",
- 		"to@gmail.com",
  		"this@gmail.com",
  		"rule@gmail.com",
  	},
  }
@mbrt
Copy link
Owner

mbrt commented Jan 13, 2022

This is not easy to do, because the diff is done at the lowest level of filters, where all the fields are simple strings. This allows for easy comparison with whatever is currently set in Gmail, without having to parse and back-translate upstream filters to higher level representations.

The line you report (to: {long@gmail.comt .... }) is effectively just a string, so the cmp package would report the same result.

@lutzky
Copy link
Contributor Author

lutzky commented Jan 13, 2022

Yep, my process definitely started with "I'll just send a pull request" and quickly turned into "this is nontrivial and should be a feature request".

@mbrt mbrt added the kind/feature New feature or request label Jan 13, 2022
@lutzky
Copy link
Contributor Author

lutzky commented Jan 13, 2022

Is there a reasonable place to say "here are all the filters before, here are all the filters after, throw it at cmp.Diff and see how bad it looks"? Should serve as a reasonable starting point.

@lutzky
Copy link
Contributor Author

lutzky commented Jan 13, 2022

Alright, I found a place and ran it, lutzky@f3264ab. The output doesn't look great.

@mbrt
Copy link
Owner

mbrt commented Jan 14, 2022

Yeah indeed. It's a bunch of strings not very different from what's existing. To make it better you really need a parser for Gmail filter expressions.

@lutzky
Copy link
Contributor Author

lutzky commented Jan 14, 2022

Yep. Now, even if I were to write such a parser, the output for cmp.Diff on imagined parsed filters doesn't look great:

 main.FilterNode{
  	And: []main.FilterNode{
  		{
  			And: nil,
  			Or: []main.FilterNode{
  				{From: "person1"},
  				{From: "person2"},
- 				{From: "person3"},
  				{From: "person4"},
  			},
  			Not:  nil,
  			From: "",
  		},
  		{
  			And: nil,
  			Or:  nil,
  			Not: &main.FilterNode{
  				And: nil,
  				Or: []main.FilterNode{
  					{From: "person5"},
  					{
  						And:  nil,
  						Or:   nil,
  						Not:  nil,
- 						From: "person6",
+ 						From: "person7",
  					},
  				},
  				Not:  nil,
  				From: "",
  			},
  			From: "",
  		},
  	},
  	Or:   nil,
  	Not:  nil,
  	From: "",
  }

Even if we hide all the nil and "" entries, what we actually want is more like this:

before: {person1 person2 person3 person4} -{person5 person6}
after:  {person1 person2 person4} -{person5 person7}
diff:
 {
   person1
   person2
-  person3
   person4
 }
 -{
   person5
-  person6
+  person7
  }

So, essentially, it's a matter of adding newlines and indentation in a few places, and running a traditional diff. I'll see if I can mock something up.

@lutzky
Copy link
Contributor Author

lutzky commented Jan 14, 2022

It's quick-and-dirty, but perhaps not entirely useless: https://go.dev/play/p/EREXFY8gTwR

@mbrt
Copy link
Owner

mbrt commented Jan 15, 2022

It's certainly a start. There are a lot more cases to consider though. From the top of my head:

  • quotes: from:"foo bar"
  • parenthesis: (a b) subject:(foo bar)
  • nested expressions: -(x {y -z} k)

And combinations of those.

@github-actions
Copy link
Contributor

This issue is stale because it has been open for 30 days without activity.
This will be closed in 7 days, unless you add the 'lifecycle/keep-alive' label or comment.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity, has become stale and will be auto-closed. label Feb 15, 2022
@mbrt mbrt added lifecycle/keep-alive Denotes an issues or PR that should never be considered stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity, has become stale and will be auto-closed. labels Feb 15, 2022
@lutzky
Copy link
Contributor Author

lutzky commented Mar 26, 2022

Tweaked it a bit to handle your example cases: https://go.dev/play/p/qNFzmtFEWCo

WDYT?

@mbrt
Copy link
Owner

mbrt commented Mar 28, 2022

Cool! I tried a couple more nasty cases (colons outside operators) and the results still seem good! https://go.dev/play/p/LtIy47e5eY7.

lutzky added a commit to lutzky/gmailctl that referenced this issue Mar 29, 2022
Makes for easier-to-read diffs.

mbrt#221
lutzky added a commit to lutzky/gmailctl that referenced this issue Apr 1, 2022
Makes for easier-to-read diffs.

mbrt#221
lutzky added a commit to lutzky/gmailctl that referenced this issue Apr 2, 2022
Makes for easier-to-read diffs.

mbrt#221
lutzky added a commit to lutzky/gmailctl that referenced this issue Apr 3, 2022
Makes for easier-to-read diffs.

mbrt#221
@mbrt mbrt closed this as completed in #239 Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request lifecycle/keep-alive Denotes an issues or PR that should never be considered stale.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants