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

Fix failed non-required checks failing overarching command #67

Merged
merged 5 commits into from Apr 26, 2022

Conversation

Green-Ranger11
Copy link
Collaborator

@Green-Ranger11 Green-Ranger11 commented Mar 14, 2022

This closes #44

If checks are not required then pull request should be mergable given the other statuses pass.
Failed checks in yellow are non-required.
Failed checks in red are required.

Which means you can get output where you can merge even though non-required checks failed.

@Green-Ranger11
Copy link
Collaborator Author

I'm going to fix the tests and try and take out the re-request logic from filterPullRequest

bin/can-merge Outdated Show resolved Hide resolved
utils/getPRWithRequiredChecks.js Outdated Show resolved Hide resolved
utils/getPRWithRequiredChecks.js Outdated Show resolved Hide resolved
Comment on lines 45 to 55
if (requiredChecks) {
if (requiredChecks.includes(node.name)) {
failure.push({ name: node.name, isRequired: true });
} else {
failure.push({ name: node.name, isRequired: false });
}
} else if (node.isRequired) {
failure.push({ name: node.name, isRequired: true });
} else {
failure.push({ name: node.name, isRequired: false });
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (requiredChecks) {
if (requiredChecks.includes(node.name)) {
failure.push({ name: node.name, isRequired: true });
} else {
failure.push({ name: node.name, isRequired: false });
}
} else if (node.isRequired) {
failure.push({ name: node.name, isRequired: true });
} else {
failure.push({ name: node.name, isRequired: false });
}
failure.push({
name: node.name,
isRequired: requiredChecks?.includes(node.name) ?? !!node.isRequired,
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is extremely way more elegant and will make the change

utils/evaluateChecks.js Outdated Show resolved Hide resolved
utils/evaluateChecks.js Outdated Show resolved Hide resolved
utils/evaluateChecks.js Outdated Show resolved Hide resolved
@Green-Ranger11
Copy link
Collaborator Author

image
image
image

@ljharb
Copy link
Owner

ljharb commented Mar 14, 2022

maybe we could indent (or add -) to the listed checks, so it's more clear what's the heading and what's the list?

also the color is nice, but it'd be great to avoid relying solely on color for conveying information (some people are colorblind), can we add a padlock in front of the required ones maybe?

@Green-Ranger11
Copy link
Collaborator Author

Ah yes those are all great points and will do that shortly!

@Green-Ranger11
Copy link
Collaborator Author

image
image

@ljharb
Copy link
Owner

ljharb commented Mar 14, 2022

Sounds good - altho if the padlock is there, maybe we don't need the hyphens; we can just use spaces to indent?

@Green-Ranger11
Copy link
Collaborator Author

Sure! Will just add the space instead because the unlocked padlock icon doesn't really match as it looks exactly like this 🔓 but the locked padlock looks exactly as is in terminal and matches all the other emojis.

@Green-Ranger11
Copy link
Collaborator Author

image
image

@Green-Ranger11
Copy link
Collaborator Author

image

bin/can-merge Outdated Show resolved Hide resolved
utils/getPRWithRequiredChecks.js Outdated Show resolved Hide resolved
utils/getPRWithRequiredChecks.js Outdated Show resolved Hide resolved
@Green-Ranger11
Copy link
Collaborator Author

Just double checking but do the checks align up on your terminal? This may look different on other terminals which is something i have not tested.
If this pr can wait I can try it out on various terminals with different fonts.

@ljharb
Copy link
Owner

ljharb commented Mar 15, 2022

i'll check next time i have a pending PR :-p

@ljharb
Copy link
Owner

ljharb commented Mar 15, 2022

Seems pretty good - i'll check on a couple others to be sure.

bin/can-merge Outdated Show resolved Hide resolved
@Green-Ranger11 Green-Ranger11 force-pushed the required-checks branch 2 times, most recently from 14d01b8 to f4a5f3b Compare April 20, 2022 21:27
@ljharb ljharb merged commit bcd84f1 into main Apr 26, 2022
@ljharb ljharb deleted the required-checks branch April 26, 2022 05:27
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.

failed non-required checks should not fail the overarching command
2 participants