search

nix-community / npmlock2nix

nixify npm based packages [maintainer=@andir]
Apache License 2.0
130 stars 42 forks source link

handle case where lockfile has no version #141

Closed zimbatm closed 2 years ago

zimbatm commented 2 years ago

This is quite common for private projects. It doesn't necessarily make sense to keep bumping a version if there is no third-party user of the project.

andir commented 2 years ago

Would be nice to have a chance to comment on a PR before you self-merge them. I actually think it would be better to move it from nix-community if we apply the same shitty YOLO mentality that exists in nixpkgs here.

gilligan commented 2 years ago

Eh.. yeah.. i am not happy about this at all. I stopped contributing to nix and nixpkgs but at least here I would expect that PRs are reviewed before they are merged.

zimbatm commented 2 years ago

sorry, I assumed this wasn't controversial and shouldn't have

andir commented 2 years ago

The change is uncontroversial and I am grateful for you fixing the issue!

We've done self-merges (or really probably just me) in the past. We've always tried to get at least another person to read through the changes to make sure they make sense to someone else. Your change was excellent and even came with a test case which is really what we insist on here.

I guess it is fine to self-merge (given tests & CI pass) after giving a reasonable amount of time for a review from another person that is involved in this project. If it is urgent you (and many others) know how to reach me.

gilligan commented 2 years ago

I want to second that i have no concerns about code quality per se. It’s a matter of process and more specifically: same process for everyone

If a change is non-controversial that should mean a review should be swift so there would be even less reason to just merge.