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

warn if the PR Author does not have valid config setup #181

Merged
merged 3 commits into from
Feb 17, 2018

Conversation

priyank-p
Copy link
Contributor

This displays a warning if PR Author does not have valid config setup using git config.

This displays a warning if PR Author does not have valid config
setup using `git config`. Before it use to just display as:

  `Author null <> (@github-user-name)`

fixes: nodejs#180
@codecov
Copy link

codecov bot commented Feb 16, 2018

Codecov Report

Merging #181 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   93.69%   93.71%   +0.01%     
==========================================
  Files          18       18              
  Lines         666      668       +2     
==========================================
+ Hits          624      626       +2     
  Misses         42       42
Impacted Files Coverage Δ
lib/pr_summary.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75f2b21...4d5485c. Read the comment docs.

@tniessen tniessen changed the title warn if the PR Author does have valid config setup warn if the PR Author does not have valid config setup Feb 16, 2018
cli.table('Author',
`${author.name} <${author.email}> (@${author.login}${authorHint})`);
} else {
// user do not have correct `git config` setup
Copy link
Member

Choose a reason for hiding this comment

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

I think the information here comes from their github account settings? The git config setup are only visible in the commit information.

Take a look at the profile page https://github.com/antoine-amara and they don't have the name nor the email setup.

Copy link
Contributor Author

@priyank-p priyank-p Feb 16, 2018

Choose a reason for hiding this comment

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

Yeah actually not too sure about i guessed it since if you look at the issues the commit the user reference have correct config of commit.

Edit: issue link nodejs/node#18662

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at the profile page https://github.com/antoine-amara and they don't have the name nor the email setup.

I don't think that matters see the issue link and the commits that are referenced they seem to perfect commit except the last one (which was the one in the PR).

Copy link
Member

@joyeecheung joyeecheung Feb 16, 2018

Choose a reason for hiding this comment

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

I don't think it's possible to get an empty git config, since git should prompt for a username and a email when you commit for the first time. So the commits should always come with a name and a email (could be totally bogus, but still). It is possible to have empty github profile settings if you don't put those down during github account registration, or delete them afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See that something i am unsure about too. I have no idea how that occurred at the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the user squashed a that commit so it might be that he changed his email causing the commit to empty details. Since it does have users profile picture.

Copy link
Member

Choose a reason for hiding this comment

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

@cPhost Hmm..maybe the user has set the email to private? I believe that would cause the email queried in the API to be null but Github still knows how to associate the commits with the account since they know about the private email address. Either way the warning should be something like "Could not retrieve the email from the PR author's GitHub profile"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, cool i can update the warning, also the name is also not present so i will add that in too.

@@ -59,6 +59,7 @@ const firstTimerPrivatePR = readJSON('first_timer_pr_with_private_email.json');
const semverMajorPR = readJSON('semver_major_pr.json');
const fixAndRefPR = readJSON('pr_with_fixes_and_refs.json');
const conflictingPR = readJSON('conflicting_pr.json');
const incorrectConfigPR = readJSON('incorrect_config_pr.json');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe renaming this to something like emptyProfilePR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah makes sense since this seems to be something other than incorrect config i was thinking.

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