pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

feat(link): add --force flag #122

Closed claytonrcarter closed 7 months ago

claytonrcarter commented 7 months ago

Hi there, now that I'm using Pulsar, I find myself with a fair number of old Atom packages that need tweaks or updates, and so I'm doing this dance a lot:

$ git clone $PACKAGE_THAT_NEEDS_A_FIX_OR_UPDATE
$ cd $PACKAGE_THAT_NEEDS_A_FIX_OR_UPDATE
$ npm i
$ ppm link
Linking $CURRENTLY_INSTALLED_PACKAGE to $PACKAGE_THAT_NEEDS_A_FIX_OR_UPDATE failed: EEXIST: file already exists, symlink '$PACKAGE_THAT_NEEDS_A_FIX_OR_UPDATE' -> '$CURRENTLY_INSTALLED_PACKAGE'

# dang it!

Obviously, the fix is simple and easy (rm -rf $CURRENTLY_INSTALLED_PACKAGE & try again), but it also felt like a --force flag would be handy to just do that for me while rerunning the command.

This was a pretty trivial change, and I'm happy to make changes as desired. I added 2 tests: one to confirm that it errors if the target exists, and another to confirm that it nukes it and then links when used with the --force flag.

Alternatives

I briefly considered calling it link --triforce. ⚔️

Thank you!

confused-Techie commented 7 months ago

Thanks a ton for contributing!

This is honestly a fantastic idea! Although my only concern is the --force flag possibly not being obvious enough in behavior. Since this flag does completely erase data on the client system. Makes me almost want to suggest we call it --destroyOriginal or something aggressive, but at the same time this should only be data from the package, and not be anything a user can't just get back via a redownload.

So I'm 100% on board with the concept, as well as love the implementation, very much appreciating the specs you've written as well. But curious if @DeeDeeG has any thoughts on the best flag name.

But overall looks awesome!

claytonrcarter commented 7 months ago

only concern is the --force flag possibly not being obvious enough

I chose --force because it reminded me of rm -f ..., git branch --force ... and some other commands that have similar flags, so it felt pretty "standard" for this sort of thing. That said, I'm totally happy to change it if another option sounds better to you and/or the team.

Thanks a ton for contributing!

You're welcome. I'm happy to help out!

savetheclocktower commented 7 months ago

I'm fuzzy on the use case here.

Assume language-foo is a Pulsar package I've got installed, and I notice something I'd like to fix. Here's what I usually do:

cd ~/Code/AtomPackages # or whatever
git clone git@github.com:foo/language-foo
cd language-foo
npm install
ppm link --dev .

Packages in ~/.pulsar/dev/packages shadow the ones in ~/.pulsar/packages — if a package exists at the former path in dev mode, it'll be loaded instead of the latter path. (This also requires that you run Pulsar in dev mode with pulsar --dev, of course.)

I think if a package is going to be removed from the directory, it should be uninstalled first, even if ppm is the one doing the uninstallation.

This isn't a veto; just want to understand it better.

confused-Techie commented 7 months ago

From my understanding the use case is essentially:

So I can absolutely see the valid use case, and it does make complete sense, since very likely you'd want to be able to track your changes on your own fork, so will need to download it again elsewhere prior to publication. While yes it could be uninstalled first, I get that it can become repetitive.

So while I think it's a total valid use case, I have to admit it's probably only valid for a small portion of the userbase, which is why I feel like a non-standard flag may be more appropriate. While it makes things easier for the population that needs it, it doesn't use something too common that people might misunderstand the effect it has.

But if you feel that it's not a widespread use enough, I'm happy to hear it out and reconsider

confused-Techie commented 7 months ago

@claytonrcarter Thanks for the feedback. It feels good to me to change it, but I'm not the most attached to this repo, so I'd much rather get the opinion of someone like I've pinged if they feel it's at all necessary.

But I do totally get that it's a perfect standard flag, which is partially why it might be good to change it, so it's not easy to do without realizing the effect. Since I don't want someone to assume it forces the linking process, not realizing it forces the deletion of a file elsewhere on the system, since the force usually means to force the action taking place, rather than some side effect of the action.

An example that kinda works for me is a command that connects to a wifi network using --force I'd assume would force the network connection, rather than forget another one because of some internal logic. But don't worry too much for now!

savetheclocktower commented 7 months ago

It's only slightly fewer keystrokes than ppm uninstall language-foo && ppm link, but I'm OK with it as long as the user understands that it's irrevocable and ppm unlink won't restore the original state.

claytonrcarter commented 7 months ago

The compelling part of this, for me, is convenience. If I run ppm link, I'm doing so because I want it to create a link. If it doesn't because I have something in the way (for whatever reason), then I want to "force" it to do so. (Like, "yes, I know what I'm doing".) Pressing up and then adding -f feels much more ergonomic than having to run rm -rf ~/.pulsar/packages/$FOO or ppm uninstall $FOO and then rerunning the link. For those reasons, I don't think that it really matters what the flag is, so long as it includes a short option. I would be fine with something like --destroyOriginal so long we also include an short flag like -d of -D, for example.

Other options

Prior art

if a package is going to be removed from the directory, it should be uninstalled first

Do you mean that you'd prefer that the user have to run ppm uninstall, or just that we invoke the code w/i ppm uninstall for the user? FWIW, the code in ppm uninstall is just running fs.removeSync(...) (from package fs-plus, I assume because fs.rmSync(targetPath, { recursive: true, force: true }) was not supported on early Node versions at the time of writing) after checking for a package.json, so this implementation is not too far off already. I can look into reusing the code in uninstall, though I wonder if the benefit will be really worth it just to wrap a call to removeSync().

requires that you run Pulsar in dev mode

I grant that this is the "blessed" path for this sort of thing, but – personally – I long ago stopped using dev mode because I found it easier and more convenient to just link my customized packages directly into ~/.pulsar/packages. As I recall, this came about because I wasn't hacking on Atom at the time, only on a few packages. And then, once I got my changes to where I wanted them, I just wanted to use them w/o having to always drop into dev mode. And not only because I wanted to test drive in real usage, but also because it can take some time before PRs are merged, and I didn't want to make my change and then abandon it for weeks or months (or ever) while I waited for a merge and release.

probably only valid for a small portion of the userbase

This is true, and it is a pretty straightforward workaround to have to manually uninstall or remove the package/dir that's in way. Adding a quick and simple way to say "yeah yeah yeah, I know what I'm doing, just create the link already" feels like a valid enough use case that it's might be worth a small DX/QOL improvement.

savetheclocktower commented 7 months ago

I don't feel strongly enough about this to keep discussing it when there are syntax highlighting fixes to do, so I'll leave you folks to it. :-)

confused-Techie commented 7 months ago

Thanks for taking a look at this one @DeeDeeG and glad to see you merged it.

Also of course thanks a ton to @claytonrcarter for your wonderful contribution and patience!