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

Include created and removed files for diffForFile, JSONDiffForFile, JSONPatchForFile #369

Merged
merged 6 commits into from
Sep 8, 2017

Conversation

bdotdub
Copy link
Contributor

@bdotdub bdotdub commented Sep 7, 2017

Functions

  • diffForFile
  • JSONPatchForFile
  • JSONDiffForFile

@orta
Copy link
Member

orta commented Sep 7, 2017

This is looking sweeeeeeet 👍

expect(empty).toEqual({
before: null,
after: after,
diff: [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orta Would love to get your thoughts here.

For created files, I think it makes sense to have before return null since the before is the absence of the file. The diff is technically correct to, but feel unexpected and/or inconsistent with other behavior.

What do you think about JSONPatchForFile returning null for before, but the diff as the result between {} and after?

So instead of this:

{
  "op": "replace",
  "path": "",
  "value": {
    "a": "o, world",
    "b": 3,
    "c": ["one", "two", "three", "four"],
  }
}

it becomes this:

[
  {
    "op": "add",
    "path": "/a",
    "value": "o, world"
  },
  {
    "op": "add",
    "path": "/b",
    "value": 3
  },
  {
    "op": "add",
    "path": "/c",
    "value": [
      "one",
      "two",
      "three",
      "four"
    ]
  }
]

While not technically correct, I think it makes more sense semantically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 👍

bdotdub added a commit to bdotdub/danger-js that referenced this pull request Sep 7, 2017
@bdotdub bdotdub force-pushed the bw/all-diffs branch 3 times, most recently from e8b8999 to bf06857 Compare September 8, 2017 02:58
@bdotdub
Copy link
Contributor Author

bdotdub commented Sep 8, 2017

@orta I think this is ready! 👌

@bdotdub bdotdub changed the title [WIP] Include created and removed files for diffForFile, JSONDiffForFile, JSONPatchForFile Include created and removed files for diffForFile, JSONDiffForFile, JSONPatchForFile Sep 8, 2017
@orta
Copy link
Member

orta commented Sep 8, 2017

Awesome, amazing and beautifully tested 👍

@orta orta merged commit f0a9252 into danger:master Sep 8, 2017
@orta
Copy link
Member

orta commented Sep 8, 2017

Shipped as Danger 2.0a15

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