Skip to content

Conversation

shine2lay
Copy link
Member

@shine2lay shine2lay commented Feb 22, 2021

see the discussion in #509 (comment)

output
image

@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #512 (07f754a) into master (7828002) will decrease coverage by 0.02%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
- Coverage   91.75%   91.72%   -0.03%     
==========================================
  Files          99       99              
  Lines        2159     2164       +5     
  Branches      406      406              
==========================================
+ Hits         1981     1985       +4     
- Misses        154      155       +1     
  Partials       24       24              
Impacted Files Coverage Δ
lib/actions/handlebars/populateTemplate.js 70.83% <0.00%> (-3.08%) ⬇️
lib/validators/approvals.js 93.75% <100.00%> (+0.27%) ⬆️

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 7828002...2b5d7c3. Read the comment docs.

teamMembers = _.union(teamMembers, owners)
let validatorContext = { name: 'approvals' }

output.push(constructOutput(validatorContext, _.union(teams, owners), limitOption, {status: 'info', description: `Only the following teams and users approval will counted`}))
Copy link
Contributor

@vsingal-p vsingal-p Feb 22, 2021

Choose a reason for hiding this comment

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

You can use "Only approvals from following sources are counted." verbiage instead
This will cover all future options in limit as well

since codeowners is also a part of limit if set to true

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use _.union(teams, owners) or approvedReviewers ?

Copy link
Member Author

@shine2lay shine2lay Feb 22, 2021

Choose a reason for hiding this comment

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

i thought input should be a list of user that can approve it rather than the current approvedReviewers that is counted

Copy link
Member Author

Choose a reason for hiding this comment

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

do you think approvedReviewers make more sense here??

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, because input is already displayed in settings field in the output

@shine2lay
Copy link
Member Author

@jusx can you please review this?

@shine2lay shine2lay requested a review from jusx February 23, 2021 16:15
@vsingal-p
Copy link
Contributor

@jusx Can you please review this?

Copy link
Member

@jusx jusx left a comment

Choose a reason for hiding this comment

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

The informational tip LGTM

@shine2lay shine2lay merged commit 219e517 into mergeability:master Mar 11, 2021
@shine2lay shine2lay deleted the info_status branch March 11, 2021 19:41
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