Implement the action #7
Conversation
dabrady
force-pushed
the
feat/initial-implementation
branch
from
5b492f6 to
eb7e65e
Compare
ewong26
left a comment
ewong26
left a comment
There was a problem hiding this comment.
Code looks good from my end. Just some minor clarification questions about syntax and the way you've modified the .eslintrc file
| describe('#requestReviewers', () => { | ||
| afterEach(jest.clearAllMocks); | ||
|
|
||
| var payload = { |
There was a problem hiding this comment.
You excluded test files from linting in the .eslintrc.js file. Is this one of those scenarios where you prefer es5 over es6? Pretty sure payload is never modified so it can be defined as a const, same for the other variables below
There was a problem hiding this comment.
Clarification, I haven't excluded test files from linting, I'm excluded them from being linted by a particular plugin which focuses on an opinionated use of arrow functions.
To your point about const vs var, const won't have the effect you think it will here. Objects are mutable: it doesn't matter what box you put them in. const simply prevents reassignment, not mutation. For that reason, I avoid using const in general, and never use const for mutable data types like objects and arrays unless I'm defining a 'shared constant', in which case I also use Object.freeze() and make the name ALL_CAPS.
(This is actually one of the few things I've ever blogged about 😆 If you're interested in reading, I have a 3-part series on DEV.to.)
There was a problem hiding this comment.
I just read the whole vars part and will give the other ones a go. Might have some questions afterwards 😅 there have been many a heated discussions on the use of var/let and const.
| } | ||
|
|
||
| // Safeguard against unsupported events. | ||
| if (context.eventName != 'pull_request') { |
There was a problem hiding this comment.
Same here, just double checking since you've allowed this on eslint. Is there a reason we're using != instead of !==?
There was a problem hiding this comment.
(As clarified above, I haven't excluded test files from linting, I'm excluded them from being linted by a particular plugin which focuses on an opinionated use of arrow functions.)
To your point of feedback about != vs !==, this is essentially the same question about == vs ===, or at least my opinions are the same. ===/!== is particularly useful when you truly care that the values and the types match, and want to entirely ignore the possibility of equivalence (e.g. in the rare case where you truly care about distinguishing between the number 2 and the numeric string "2").
In this case here, if the types are different, we'd always want to fail. So you could argue that that ===/!== is technically a better choice. But in reality, there is no value of the context.eventName variable except the exact string "pull_request" that would ever make this conditional truthy, and so the choice of tool to use here is essentially a personal one. And my reason for choosing != over !== in this particular case is that it is one fewer characters to type 😁
ewong26
left a comment
ewong26
left a comment
There was a problem hiding this comment.
lgtm, thought my first approval would make it passed all the updates 😅
| "scripts": { | ||
| "lint": "eslint .", | ||
| "prepare": "ncc build index.js -o dist --source-map --license licenses.txt", | ||
| "build": "ncc build index.js -o dist --source-map --license licenses.txt", |
Contributes to ENG-81
ℹ️ Please see the project README for the overall documentation and design of this GitHub Action: https://github.com/ProdPerfect/ghaction-nodiff/blob/main/README.md
This PR contains the initial implementation of this GitHub Action, which identifies files in change sets that are essentially "no different" from what they were before they were changed and lets you respond to such occurrences in a few predefined ways. Right now, "no difference" refers exclusively to whitespace changes, but I intend it to be intentionally broad to encompass future enhancements to that definition.
I used a draft PR (#3) while iterating on this as a sort of meta-level integration test, and was able to test it quite thoroughly as a result.
(fyi @Detect)