moonrepo / starbase

Framework for building performant command line applications and developer tools.
MIT License
78 stars 6 forks source link

feat: support shell value quoting #84

Closed varshith257 closed 3 months ago

varshith257 commented 3 months ago

fixes #81 /claim #81

Here are the docs I have used as a reference for adding quoting rules:

varshith257 commented 3 months ago

@milesj I have added escaping/quoting rules for the info I have researched for the respective shells.

It will be great to review them. If any suggestions or modifications are needed, I will incorporate them as needed :)

varshith257 commented 3 months ago

IMO, for the format_env_unset and format_path_setmethods, the quote method is not directly needed because these methods are typically used for operations where quoting is not necessary or it is handled implicitly by the shell environment.

varshith257 commented 3 months ago

@milesj Can you chime in for review?

milesj commented 3 months ago

@varshith257 I'll be out most of the weekend, but in the mean time, could you link all the docs you used as references, and add some tests cases. When I'm back, I'll review it.

varshith257 commented 3 months ago

Sure

varshith257 commented 3 months ago

@milesj Hope you consider my work and my last 3 days' efforts on research and made this progress except for the small fixes left. I am more interested in working on this issue and to gain exposure and experience with the various shells.

I have incorporated the most shell-specific quoting rules to each shell and left unquoted paths and I am not sure paths needed quoting. If needed, I will incorporate them. Please review and if any changes are needed I am quick here to implement and complete this issue.

varshith257 commented 3 months ago

@milesj I have added the reviewed suggestions and also fixed the failing tests

varshith257 commented 3 months ago

@milesj Any comments on my replies to the reviews? Let's quickly wrap it up and complete this :)

varshith257 commented 3 months ago

And the tests are failing due to snapshots missing of Nu shell from macOS and Ubuntu. I have done this work in Windows. I think we need to run tests on Ubuntu and macOs to generate respective snapshots which is causing test fails

varshith257 commented 3 months ago

@milesj I have removed the quote method in unset methods wherever not needed.

milesj commented 3 months ago

Look into fixing the lints, and I'll fix the snapshots.

varshith257 commented 3 months ago

@milesj Lints also the same of failed for snapshots will get it resolved once it generates snapshots for Nu shell of Ubuntu and macOS

milesj commented 3 months ago

Can you fix the warnings?

varshith257 commented 3 months ago

There are 30+ warnings emitted could be another effort to reiterate and test them, but I will fix the warnings too to make our codebase clean and improve code quality

varshith257 commented 3 months ago

@milesj I have fixed all the lint warnings emitted and ensured all tests passed

The snapshots generation are only left for now :)

milesj commented 3 months ago

/approve

algora-pbc[bot] commented 3 months ago

@milesj: The claim has been successfully added to reward-all. You can visit your dashboard to complete the payment.

milesj commented 3 months ago

Thanks for this, much appreciated.

I'll merge through the error and fix on master.

milesj commented 3 months ago

/tip $100 @varshith257

Thanks for all the work! There will definitely be more in the future.

algora-pbc[bot] commented 3 months ago

👉 @milesj: Navigate to your dashboard to proceed

algora-pbc[bot] commented 3 months ago

🎉🎈 @varshith257 has been awarded $100! 🎈🎊