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 rambda@0.5.1 w/ npm auto-update #10487

Merged
merged 1 commit into from Feb 7, 2017
Merged

Conversation

hare1039
Copy link
Contributor

@hare1039 hare1039 commented Jan 31, 2017

Pull request for issue: #10381
Related issue(s): # #

@cdnjs/intern3 Please help me reviewing this PR. Thank you.
Note that the author want only the webVersion.js, and this file doesn't appear before 0.3.1.

Checklist for Pull request or lib adding request issue follows the conventions.

Note that if you are using a distribution purpose repository/package, please also provide the url and other related info like popularity of the source code repo/package.

Profile of the lib

  • Git repository (required): https://github.com/selfrefactor/rambda
  • Official website (optional, not the repository):
  • NPM package url (optional): https://www.npmjs.com/package/rambda
  • License and its reference: MIT, ref
  • GitHub / Bitbucket popularity (required):
    • Count of watchers: 1
    • Count of stars: 52
    • Count of forks: 0
  • NPM download stats (optional):
    • Downloads in the last day: 120
    • Downloads in the last week: 431
    • Downloads in the last month: 1206

Essential checklist

  • I'm the author of this library
    • I would like to add link to the page of this library on CDNJS on website / readme
  • This lib was not found on cdnjs repo
  • No already exist / duplicated issue and PR
  • The lib has notable popularity
    • More than 100 [Stars / Watchers / Forks] on [GitHub / Bitbucket]
    • More than 500 downloads stats per month on npm registry
  • Project has public repository on famous online hosting platform (or been hosted on npm)

Auto-update checklist

  • Has valid tags for each versions (for git auto-update)
  • Auto-update setup
  • Auto-update target/source is valid.
  • Auto-update filemap is correct.

Git commit checklist

  • The first line of commit message is less then 50 chars, be clean and clear, easy to understand.
  • The parent of the commit(s) in the PR is not old than 3 days.
  • Pull request is sending from a non-master branch with meaningful name.
  • Separate unrelated changes into different commits.
  • Use rebase to squash/fixup dummy/unnecessary commits into only one commit.
  • Close corresponding issue in commit message
  • Mention related issue(s), people in commit message, comment.

Copy link
Contributor

@sufuf3 sufuf3 left a comment

Choose a reason for hiding this comment

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

I think you still need rambda.js, because the webVersion.js would call function from rambda.js

@hare1039
Copy link
Contributor Author

@sufuf3 But in the readme said include the webVersion.js for production. So I don't think rambda.js is necessary.

Copy link
Contributor

@clairetsai818 clairetsai818 left a comment

Choose a reason for hiding this comment

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

You need to minify the source file.

@hare1039
Copy link
Contributor Author

hare1039 commented Feb 1, 2017

@clairetsai818 I can't minify the webVersion.js using minify.sh. The file seemed already minified.

@sufuf3
Copy link
Contributor

sufuf3 commented Feb 1, 2017

@hare1039 But with index.js and webVersion.js, they both has the "function add(a,b){if(b===void 0){return c=>add(a,c);" ... etc.
and index.js need rambda.js
What do you think?
and there are some old versions not exist webVersion.js ...
I have no idea about that should you have to take index.js also?

@hare1039
Copy link
Contributor Author

hare1039 commented Feb 1, 2017

@sufuf3 Okey, let me ask the author.

@hare1039
Copy link
Contributor Author

hare1039 commented Feb 1, 2017

@sufuf3 @clairetsai818 The author said

webVersion.js is bundled and ready to use. It doesn't depend on other packages.

And the older versions 0.1.0-0.3.1

  • index.js is the minified file
  • rambda.js is the unminified file
    Note 0.1.0-0.1.2 has another file structure (with no index.js). But when minifing rambda.js occurs errors(Unexpected token: operator (>)), so I don't include them.

Please help me reviewing this PR again. Thank you.

Copy link
Contributor

@extend1994 extend1994 left a comment

Choose a reason for hiding this comment

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

@hare1039 You can notice that there are minified file rambda.min.js in v0.1.1 and v0.1.2, it seems like it's a part of rambda.js. I think you should ask the author about the minified files' problem or look for some help from your mentor rather than just not to include the related files.

@hare1039
Copy link
Contributor Author

hare1039 commented Feb 2, 2017

After some discussion, we don't need to include files before 0.3.1.
So I removed the commit of adding old version.
@cdnjs/intern3 Please review this PR again. Thank you.

Copy link
Contributor

@clairetsai818 clairetsai818 left a comment

Choose a reason for hiding this comment

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

Maybe you should mention this PR in the issue you discuss with the author, and we can see the conversation.

@hare1039
Copy link
Contributor Author

hare1039 commented Feb 3, 2017

@clairetsai818 We discussed in #10381. Please take a look of it.
@pvnr0082t Please help me reviewing this PR. Thank you.

"name": "self_refactor",
"url": "https://github.com/selfrefactor"
},
"license": "MIT",
Copy link
Contributor

Choose a reason for hiding this comment

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

@hare1039 I think you should notify author that the license should be changed to MIT because the package.json file on both GitHub repo and NPM shows ISC. Please double confirm this. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pvnr0082t The author changed to MIT in package.json https://github.com/selfrefactor/rambda/blob/master/package.json#L22
Please confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hare1039 Nice! Please rebase onto the latest master branch. Thanks.

Copy link
Contributor Author

@hare1039 hare1039 Feb 6, 2017

Choose a reason for hiding this comment

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

@pvnr0082t I updated the branch! Please confirm.

Copy link
Contributor

@pvnr0082t pvnr0082t left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

The commit message is too long ... I thought we taught this at the very beginning ...

@hare1039
Copy link
Contributor Author

hare1039 commented Feb 7, 2017

I missed a blank line between the title and the detail. Now I fixed it. Please review this PR again. Thank you.

Copy link
Contributor

@pvnr0082t pvnr0082t left a comment

Choose a reason for hiding this comment

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

LGTM

@PeterDaveHello PeterDaveHello merged commit 8056f39 into cdnjs:master Feb 7, 2017
@hare1039 hare1039 deleted the rambda branch February 8, 2017 14:26
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.

None yet

6 participants