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

1235 notify third parties for changes in the license book #1305

Merged

Conversation

barmac
Copy link
Collaborator

@barmac barmac commented Mar 14, 2019

Closes #1235

@barmac barmac requested a review from philippfromme March 14, 2019 13:51
@ghost ghost assigned barmac Mar 14, 2019
@ghost ghost added the needs review Review pending label Mar 14, 2019
@barmac
Copy link
Collaborator Author

barmac commented Mar 14, 2019

Note for reviewer: We use diff package due to broken behaviour of Node's child_process module when diffing big different (edited) files.

@barmac barmac force-pushed the 1235-notify-third-parties-for-changes-in-the-license-book branch from 7e08528 to 819b1ae Compare March 14, 2019 14:21
@barmac
Copy link
Collaborator Author

barmac commented Mar 14, 2019

@philippfromme ready.

@nikku
Copy link
Member

nikku commented Mar 14, 2019

We use diff package due to broken behaviour of Node's child_process module when diffing big files.

What is the broken behavior and why / how does it apply to setup?

@barmac
Copy link
Collaborator Author

barmac commented Mar 14, 2019

Previously, I tried to use execa.sync to generate diff:

const exec = require('execa').sync;

const diff = exec('diff', ['-u', '-', file], { input: anotherFile }).stdout;

However, it always throws error when the files differ. diff simply exits with 1 for unidentical files and it's considered as error for child_process and execa 🙄. Cf. nodejs/node#19494 (comment).

Anyway, the alternative would be to read stdout from error which feels hacky for me. So instead of these guys we use npm package for diff.

@nikku
Copy link
Member

nikku commented Mar 15, 2019

Thanks for the note.

I'd personally prefer built-in utilities, especially if the alternative is to require another dependency that is really only required on the CI.

Just to understand correctly, what we'd need is something like this, right?

const exec = require('execa').sync;

async function computeDiff(file, otherFile) {

  // diff exits with <1> if a diff exists; we must account for that special behavior
  // cf. https://github.com/nodejs/node/issues/19494#issuecomment-374721063
  try {
    return exec('diff', ['-u', '-', file], { input: otherFile }).stdout;
  } catch (e) {

    if (e.code !== 1) {
      throw e;
    }
    
    return e.stdout;
  }
}

@philippfromme
Copy link
Contributor

Should I review this or not?

@barmac
Copy link
Collaborator Author

barmac commented Mar 15, 2019

@nikku Correct, this is more or less what I wrote before.

@philippfromme I'll include Nico's suggestion and then it's ready. Just a sec.

@barmac barmac force-pushed the 1235-notify-third-parties-for-changes-in-the-license-book branch from 6f8140b to 8d19965 Compare March 15, 2019 13:00
@barmac
Copy link
Collaborator Author

barmac commented Mar 15, 2019

@philippfromme ready

Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

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

🐎

@merge-me merge-me bot merged commit 4c2a55d into master Mar 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the 1235-notify-third-parties-for-changes-in-the-license-book branch March 15, 2019 14:17
@ghost ghost removed the needs review Review pending label Mar 15, 2019
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.

3 participants