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 i18next-xhr-backend@1.4.2 w/ npm auto-update #11447

Merged
merged 1 commit into from
Sep 14, 2017

Conversation

dakshshah96
Copy link
Contributor

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

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

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

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@dakshshah96 congratulations! 0b2407d CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9487, thank you 😀

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 this PR is fine. Just need to tag the author to notice him. Could you add cc @jamuhl in the commit message?

Although there are more versions from GitHub then npm.
But I think it's OK because the git tag v0.3.0 is duplicate with 0.3.0.
If manually add v0.0.2 from GitHub is better.
(npm release v0.0.1, but GitHub repo didn't release this version)

@dakshshah96
Copy link
Contributor Author

@sufuf3 Can you please clarify that? I didn't get what you meant about the manual additions.

@sufuf3
Copy link
Contributor

sufuf3 commented Jun 25, 2017

@dakshshah96 Sorry about that.

  1. Could you cc @jamuhl in the commit message to notice author?
  2. Could you manually add v0.0.2 from GitHub in another commit? Let CDNJS host v0.0.2 because npm auto-update can't get this version.

Thank you.

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@dakshshah96 congratulations! fcc555d CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9526, thank you 😀

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@dakshshah96 congratulations! ef1859f CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9527, thank you 😀

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@dakshshah96 congratulations! d6d2a8d CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9529, thank you 😀

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@dakshshah96 congratulations! 949dcb0 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9528, thank you 😀

@dakshshah96
Copy link
Contributor Author

@sufuf3 Done 👍

@sufuf3 sufuf3 self-requested a review June 25, 2017 14:24
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.

LGTM

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.

@dakshshah96
I'm sorry. I found that the versions before v0.0.4 should manually add due to the different file structure.
Would you add them?
Thank you.

Sorry. I didn't notice that.

@sufuf3 sufuf3 self-requested a review June 25, 2017 14:33
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.

I don't think that the author packaged the versions before v0.0.4, which means that the users can't use these versions directly, but have to use some commands to get the correct files, e.g gulp build (source: https://github.com/i18next/i18next-xhr-backend/tree/0.0.3#i18next-xhr-backend).
In a conclusion, we don't need to manually add them. If we need, I think we should ask the author can he/she provide the files.

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@dakshshah96 congratulations! 67f6b29 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/12493, thank you 😀

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.

As @extend1994 mention #11447 (review)
Then this PR LGTM

Thanks.

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.

@sufuf3 Please remove your manual adding of v0.0.2, thanks.
Another reference: similar library i18next-browser-languagedetector.
You can see the same situation in v0.0.10 ~ v0.0.5. They are not hosted by CDNJS.

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@dakshshah96 congratulations! 0f9205e CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/12531, thank you 😀

@sufuf3
Copy link
Contributor

sufuf3 commented Sep 12, 2017

@extend1994 I removed it, please help review this PR, thanks.

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.

LGTM

@PeterDaveHello PeterDaveHello merged commit bf2e689 into cdnjs:master Sep 14, 2017
@ghost ghost removed the in progress label Sep 14, 2017
@PeterDaveHello
Copy link
Contributor

Thanks all!

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.

5 participants