nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

todo: warn about conflicts #68

Closed joyeecheung closed 6 years ago

joyeecheung commented 6 years ago

I guess the lander will notice this when they push...but a warning in advance wouldn't hurt.

alopezsanchez commented 6 years ago

Hello. I can take it if you don't have any inconvenience.

@joyeecheung can you please be more specific with the feature? :smiley: Maybe I need a guidance on this. Thank you!

priyank-p commented 6 years ago

@alopezsanchez if you want to work on it here are couple of things you need to do

Hope this helps.

joyeecheung commented 6 years ago

+1 to what @cPhost says, though I think the property would appear in this.commits[i].commit.mergeable

priyank-p commented 6 years ago

@joyeecheung yes it should appear in this.commits[i].commit.mergeable

alopezsanchez commented 6 years ago

Thank you for your advices. I'm taking a look at Github's GraphQL API and the property mergeable is at pullRequest level (https://developer.github.com/v4/reference/object/pullrequest/).

So.. what is the goal of this issue? Just warn when the PR mergeable state is CONFLICTING? Do you want to know the files and the commits affected?

In case of the second, how I can obtain these information? Sorry, but I don't have much experience with GraphQL API and I don't find anything in the search engine.

Thank you!!

priyank-p commented 6 years ago

In case of the second, how I can obtain these information? Sorry, but I don't have much experience with GraphQL API and I don't find anything in the search engine.

I agree that the documentation of GitHub GraphQL is not the best. here is the link https://developer.github.com/v4/reference/enum/mergeablestate/

And i don't think you can get the list of files that would be conflicting. I might be wrong!

alopezsanchez commented 6 years ago

If I can't get the conflicting commits, then I don't understand why you said mergeable property would appear in this.commits[i].commit.mergeable.

For this, mergeable should be in queries/PR.gql.

Recapitulating, this feature is just about warning if the PR state is CONFLICTING. We don't want to know about conflicting commits and files, right?

What do you think?

priyank-p commented 6 years ago

@alopezsanchez if a commit is conflicting means the whole PR is conflicting. If the state mergable is CONFLICTING then the commit is conflicting.

Does this help?

alopezsanchez commented 6 years ago

Yes, but if a PR has n commits and there is a conflict, all of it will have conflicting state. Do we want to know which is the specific one that causes the conflict?

priyank-p commented 6 years ago

@alopezsanchez not really i think what @joyeecheung wanted was they know in advance that there is going to be conflict. I think she just wants

a warning in advance

as the lander would know later know details about it when they push think of it as a sort of heads up there is going to be a conflict.

joyeecheung commented 6 years ago

@alopezsanchez The Github GraphQL API does not seem to support reading PR diffs/files at the moment, this is also why we still have not finished the last todo in our README:

 Check number of files changed (request pre-backport)

In this case before we integrate v3 API into this project, we can can just warn about there is a conflict, the lander would know where the conflicts are when they do a rebase. (Also I don't really think it's necessary to show them the specific conflicts on our end because git rebase would do a much better job)