Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add local-lib for easy caching #28

Merged
merged 37 commits into from
Apr 8, 2024
Merged

Add local-lib for easy caching #28

merged 37 commits into from
Apr 8, 2024

Conversation

JJ
Copy link
Contributor

@JJ JJ commented Apr 5, 2024

Mainly would close #2. Also general ♻️ for JS code, and add a linter for future checks.

Copy link
Contributor Author

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few additional comments, just in case.

@@ -1,7 +1,7 @@
{
"name": "install-with-cpanm",
"version": "1.0.0",
"description": "[![Actions Status](https://github.com/perl-actions/install-with-cpanm/workflows/check/badge.svg)](https://github.com/perl-actions/install-with-cpanm/actions)",
"version": "1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to change version here to the last one, 1.5?

`-v` is for verbosity


## License
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this license for clarity, since I was forking. You might want either to delete it, or to expand collaborator names from nicks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed we might prefer a more generic Perl'ish license

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you will need to change the short license name in package.json, assuming that artistic-2.0 is compatible with it. I guess ISC (the one here, which I just transcribed, keeping it in package.json) is the license in the boilerplate originally used, but it's pretty permissive, so I guess it's not a problem to change it. IIRC, the artistic will need to add a LICENSE file too here.

README.md Show resolved Hide resolved
@oalders
Copy link
Member

oalders commented Apr 5, 2024

There's a lot of commit history here. 😅

Copy link
Member

@atoomic atoomic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.
Note squashing all these changes as one would have been fine too.
I can see multiple ideas like s/var/let/g or other that could have their own commit
but in the end it s seems pretty good change.

The main questionable part is the license.
thanks for submitting it

README.md Show resolved Hide resolved
`-v` is for verbosity


## License
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed we might prefer a more generic Perl'ish license

Copy link
Contributor Author

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original license in line 17 of package.json

`-v` is for verbosity


## License
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you will need to change the short license name in package.json, assuming that artistic-2.0 is compatible with it. I guess ISC (the one here, which I just transcribed, keeping it in package.json) is the license in the boilerplate originally used, but it's pretty permissive, so I guess it's not a problem to change it. IIRC, the artistic will need to add a LICENSE file too here.

@atoomic
Copy link
Member

atoomic commented Apr 8, 2024

@JJ Would you mind squashing and force push the change?
thanks

@JJ
Copy link
Contributor Author

JJ commented Apr 8, 2024

Not really, but you know that you can do that with a single click when merging, right? Click on the triangle to display the merging options, and then select "Squash and merge". You can then edit or delete individual commit messages...

@atoomic atoomic merged commit c70194e into perl-actions:master Apr 8, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make caching of installed modules easier
3 participants