search

electron / node-abi

:turtle: :rocket: Get the Node.js and Electron ABI for a given target and runtime
https://www.npmjs.com/node-abi
MIT License
164 stars 58 forks source link

fix: pin Node 17 to 17.0.1 #119

Closed lujjjh closed 2 years ago

lujjjh commented 2 years ago

Fixes #114.

ckerr commented 2 years ago

@malept ping

malept commented 2 years ago

General note: we need to add the semantic commit action to this repo

lujjjh commented 2 years ago

@malept Thank you for your review! :)

I've added some tests and removed the nullish coalescing operator. Please take another look.

malept commented 2 years ago

For future reference, squashing your commits as you make them in an open PR makes it difficult for reviewers to see what's changed since the last time they reviewed it. This repo should default to "squash and merge" so having the PR author do it is unnecessary.

lujjjh commented 2 years ago

Then I recommend making "squash and merge" the only option for this repository. The default merging option for this repo is not visible for contributors. If they notice that the recent PRs have not been squashed, they will manually squash their commits.

malept commented 2 years ago

Then I recommend making "squash and merge" the only option for this repository. The default merging option for this repo is not visible for contributors. If they notice that the recent PRs have not been squashed, they will manually squash their commits.

I don't really see how that's helpful here? Only users who have merge rights can actually see which merge style is being used.

lujjjh commented 2 years ago

Only users who have merge rights can actually see which merge style is being used.

That's the point. Personally, I review the commit history before contributing and adhere to the repo's style. If the repo consistently uses a squash-style merging, I won't have to squash commits manually. However, this repo's merge style is not that consistent, and I only checked the most recent two PRs before thinking, "Oh, I'll have to squash commits manually." Making "squash and merge" the only option is quite beneficial for consistency.

malept commented 2 years ago

I don't have access to change the merge config so I made a PR for the contributing docs instead (which folks should be reading anyway)

malept commented 2 years ago

Although my point still stands - force pushing in the middle of a PR review is difficult for reviewers to see changes without having to re-review an entire PR. In this case the PR isn't large, but in other cases it can be bad. (For example, I reviewed a 15,000 line PR today and would be unhappy if that PR creator decided to force push the changes they made based on my comments.)

lujjjh commented 2 years ago

This is kinda slippery slope argument. I didn't see any problem with amending the commit in this situation. Anyway, you may click the Compare button in the force push tip to see what's changed.

Do you have any other suggestions on the code?

lujjjh commented 2 years ago

I'm closing this pull request because a) I've switched to Node-API and b) it appears that no one is actively reviewing it for the moment.

Feel free if someone wants to continue working on this issue.

malept commented 2 years ago

Feel free if someone wants to continue working on this issue.

Can't really do that if the branch gets deleted :man_shrugging: