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.

Sign up for GitHub

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

Jump to bottom

[#25] Change order of arguments #27

Merged
merged 7 commits into from
Mar 26, 2019
Merged

[#25] Change order of arguments #27

merged 7 commits into from
Mar 26, 2019
+25 −18

Conversation

RumyantsevMichael
Copy link
Contributor

I have changed an order of parameters according to #25. Also, I've added a d.ts file right in the repo, so once the PR is approved it would be great to deprecate the existing typings as described here.

Change order of arguments

Verified

This commit was signed with the committer’s verified signature.
dtinth Thai Pangsakulyanont
SSH Key Fingerprint: 55555OI8FCoijciXz7UtfCxlh+psQcjsy0TFCpWoZUI
Verified
Learn about vigilant mode
7563dd4
@RumyantsevMichael RumyantsevMichael mentioned this pull request Jan 4, 2019
Closed
timche
timche requested changes Feb 18, 2019
View reviewed changes
Copy link
Member

@timche timche left a comment

Choose a reason for hiding this comment

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

Great work! 👏

Few minor things and I'm happy to release this ASAP.

P.S. Sorry for letting you wait for so long.

README.md Outdated

[![Build Status](https://travis-ci.org/redux-utilities/reduce-reducers.svg?branch=master)](https://travis-ci.org/redux-utilities/reduce-reducers)
[![npm Version](https://img.shields.io/npm/v/reduce-reducers.svg)](https://www.npmjs.com/package/reduce-reducers)
[![npm Downloads Monthly](https://img.shields.io/npm/dm/reduce-reducers.svg)](https://www.npmjs.com/package/reduce-reducers)
Copy link
Member

Choose a reason for hiding this comment

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

The badges are somehow now each in a separate line. Please revert them being all inline please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

README.md Outdated

#### What is the difference between `reduceReducers` and `combineReducers`?

This StackOverflow post explains it very well: https://stackoverflow.com/a/44371190/5741172
Copy link
Member

Choose a reason for hiding this comment

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

For some reason this whole readme is displayed as a diff. I think there is something wrong with the encoding. Can you please check that it's UTF-8?

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 don't realize what causes the problem yet

@RumyantsevMichael
Fix types according to code review.

Verified

This commit was signed with the committer’s verified signature.
dtinth Thai Pangsakulyanont
SSH Key Fingerprint: 55555OI8FCoijciXz7UtfCxlh+psQcjsy0TFCpWoZUI
Verified
Learn about vigilant mode
91fa90c
@lwolle
Copy link

lwolle commented Mar 15, 2019

Hey @timche @RumyantsevMichael, I was curious if there are blockers with this issue and if I can offer some help?

@timche
Copy link
Member

timche commented Mar 15, 2019

Actually not @lwolle . I just wasn't aware that I should re-review. Anyway, there are still some CI errors to fix before I can review.

@lwolle
Copy link

lwolle commented Mar 18, 2019

Thanks for the fast reply. I'm just waiting for this PR to go through.

@RumyantsevMichael
Copy link
Contributor Author

Actually not @lwolle . I just wasn't aware that I should re-review. Anyway, there are still some CI errors to fix before I can review.

To be honest, I don't understand what is happening in Travis. Locally both prettier and xo work perfect.

@lwolle
Copy link

lwolle commented Mar 25, 2019 •
edited
Loading

Guys, sorry being annoying. But is there any chance to get this through as soon as possible? Would you mind if I have a look, or @timche would you mind? I have no experience with the github integrated build pipelines, but would like to support.

@timche
Copy link
Member

timche commented Mar 25, 2019

@RumyantsevMichael can you please run yarn format to format src/index.js. This should resolve the issue.

timche
timche requested changes Mar 25, 2019
View reviewed changes
README.md Outdated
[![Build Status](https://travis-ci.org/redux-utilities/reduce-reducers.svg?branch=master)](https://travis-ci.org/redux-utilities/reduce-reducers)
[![npm Version](https://img.shields.io/npm/v/reduce-reducers.svg)](https://www.npmjs.com/package/reduce-reducers)
[![npm Downloads Monthly](https://img.shields.io/npm/dm/reduce-reducers.svg)](https://www.npmjs.com/package/reduce-reducers)
[![Build Status](https://travis-ci.org/redux-utilities/reduce-reducers.svg?branch=master)](https://travis-ci.org/redux-utilities/reduce-reducers)[![npm Version](https://img.shields.io/npm/v/reduce-reducers.svg)](https://www.npmjs.com/package/reduce-reducers)[![npm Downloads Monthly](https://img.shields.io/npm/dm/reduce-reducers.svg)](https://www.npmjs.com/package/reduce-reducers)
Copy link
Member

Choose a reason for hiding this comment

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

There are still unneeded changes here. Can you please run git checkout master README.md -p and revert this.

@RumyantsevMichael
Empty commit. It Should rerun CI checks

Verified

This commit was signed with the committer’s verified signature.
dtinth Thai Pangsakulyanont
SSH Key Fingerprint: 55555OI8FCoijciXz7UtfCxlh+psQcjsy0TFCpWoZUI
Verified
Learn about vigilant mode
63152a7
@RumyantsevMichael
Revert README.md

Verified

This commit was signed with the committer’s verified signature.
dtinth Thai Pangsakulyanont
SSH Key Fingerprint: 55555OI8FCoijciXz7UtfCxlh+psQcjsy0TFCpWoZUI
Verified
Learn about vigilant mode
410c464
@RumyantsevMichael
Change line endings

Verified

This commit was signed with the committer’s verified signature.
dtinth Thai Pangsakulyanont
SSH Key Fingerprint: 55555OI8FCoijciXz7UtfCxlh+psQcjsy0TFCpWoZUI
Verified
Learn about vigilant mode
74fcdd8
@RumyantsevMichael
Change loglavel of prettier and add --prettier flag for xo

Verified

This commit was signed with the committer’s verified signature.
dtinth Thai Pangsakulyanont
SSH Key Fingerprint: 55555OI8FCoijciXz7UtfCxlh+psQcjsy0TFCpWoZUI
Verified
Learn about vigilant mode
28c530e
@RumyantsevMichael
Copy link
Contributor Author

Hmm, interesting. I've installed yarn and now prettier works just like in Travis. But actually it works wrong because now it ignores the printWidth parameter and makes lines longer than 80 characters.

@RumyantsevMichael
Copy link
Contributor Author

@timche, please have a look at it.

@timche
Copy link
Member

timche commented Mar 26, 2019

Looks good now, thanks a lot! 👏

@timche timche merged commit 508df6a into redux-utilities:master Mar 26, 2019
@lwolle
Copy link

lwolle commented Mar 26, 2019

Thank you guys! 🏆

@timche
Copy link
Member

timche commented Mar 26, 2019 •
edited
Loading

Released in v1.0.0

@RumyantsevMichael RumyantsevMichael deleted the issue/25 branch March 26, 2019 13:06
@RumyantsevMichael
Copy link
Contributor Author

Oops, sorry guys. I forgot to add index.d.ts into the files section of the package.json so now it's not in a package.
I will fix it soon.

@timche
Copy link
Member

timche commented Mar 26, 2019

@RumyantsevMichael thanks for noticing. Fixed in v1.0.1

@vhenzl vhenzl mentioned this pull request Apr 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timche timche

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@RumyantsevMichael @lwolle @timche