vuki656 / package-info.nvim

✍️ All the npm/yarn/pnpm commands I don't want to type
GNU General Public License v3.0
480 stars 27 forks source link

feat: support pnpm #91

Closed mtoohey31 closed 2 years ago

mtoohey31 commented 2 years ago

Hello! This PR adds support for pnpm, I believe I've covered all the places where changes are necessary but let me know if I missed anything. I've tested every command except change_version because I'm running into issues with that, I believe they're unrelated because it looks like it's creating two version select windows or something like that, not sure, but more research is required before I open an issue for that.

Closes #76. As I mentioned in that issue, we can just use npm outdated --json to check outdated, since pnpm doesn't support it yet, but we should keep an eye on the issue in the pnpm repo and probably allow it to use pnpm when it's supported.

mtoohey31 commented 2 years ago

Oh also, do you want me to add pnpm to the docs everywhere, or just to the parts where it explains what command it runs?

vuki656 commented 2 years ago

Really cool, thanks for the contribution.

Please do add docs so people know it's supported.

Regarding the change_version, can you describe a bit more of what happens?

As for the pnpm --outdated --json, after this is merged, I'll open an issue to track it so once pnpm supports it we can switch.

mtoohey31 commented 2 years ago

Please do add docs so people know it's supported.

Will do!

Regarding the change_version, can you describe a bit more of what happens?

Here's a screenshot: 2021-11-06T12:07:36,005557426-04:00

After I select the result in either window, I get the following error:

E5108: Error executing lua Vim:E475: Invalid argument: expected String or List

As for the pnpm --outdated --json, after this is merged, I'll open an issue to track it so once pnpm supports it we can switch.

Sounds good!

mtoohey31 commented 2 years ago

Actually, my bad, it turns out I did make a mistake and the error resulting after selecting either window was a bug in my new code for pnpm, so I'll fix that. I'm pretty sure the dual windows is another unrelated issue though, because it also happens in npm projects. I'll look into it!

mtoohey31 commented 2 years ago

Ok, pnpm-related issues are fixed, and docs are updated! Like I said, I'll try and look into the duplicate windows issue, but I believe it's unrelated to these changes since it also happens with npm.

vuki656 commented 2 years ago

Ok, pnpm-related issues are fixed, and docs are updated! Like I said, I'll try and look into the duplicate windows issue, but I believe it's unrelated to these changes since it also happens with npm.

Does that happen every time when you open the change_version popup?

mtoohey31 commented 2 years ago

My appologies, the double window bug was a result of a failed attempt on my part to solve #86, as you can see by my comment on your issue, I had completely misunderstood what that check regarding the value being "" was doing. I believe I've come up with a better fix though, so maybe we can take a look at that in a separate PR, but afaik this one is good to go.

Sorry again, I should've been working on that on a separate branch instead of trying to do multiple things at once, lesson learned.

vuki656 commented 2 years ago

No problemo :)

I'm a bit busy at the moment but feel free to open a PR and I'll take a look within a few days.

mtoohey31 commented 2 years ago

No worries, it's open at #92 when you have time.

vuki656 commented 2 years ago

Hey, I just added some tests for command retrieval and loading status. Pretty straightforward stuff.

Could you please merge master into your branch and add tests for pnpm command retrieval. Let me know if you need any help with that.

mtoohey31 commented 2 years ago

Sure, that's done and seems to be working.

vuki656 commented 2 years ago

Just merged #92, is this ready as well?

mtoohey31 commented 2 years ago

I believe so! I've been using it since I wrote it, and haven't run into any more issues.

vuki656 commented 2 years ago

I believe so! I've been using it since I wrote it, and haven't run into any more issues.

Cool, thanks for the PR.