-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
e07d15e
to
194d0e9
Compare
I'm going to fix the tests and try and take out the re-request logic from |
194d0e9
to
d892910
Compare
utils/evaluateChecks.js
Outdated
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 }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, | |
}); |
There was a problem hiding this comment.
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
d892910
to
89021b3
Compare
0b8b138
to
fe3667d
Compare
maybe we could indent (or add 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? |
Ah yes those are all great points and will do that shortly! |
fe3667d
to
b2a20cf
Compare
Sounds good - altho if the padlock is there, maybe we don't need the hyphens; we can just use spaces to indent? |
b2a20cf
to
ae89d04
Compare
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. |
ae89d04
to
f02442e
Compare
f02442e
to
a8a1fb3
Compare
7f6cdb7
to
31c74ba
Compare
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. |
i'll check next time i have a pending PR :-p |
Seems pretty good - i'll check on a couple others to be sure. |
3930161
to
85125ab
Compare
85125ab
to
f903b41
Compare
14d01b8
to
f4a5f3b
Compare
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.