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

Fixed quote types inconsistency in a warning message #2297

Merged
merged 1 commit into from
Oct 6, 2017

Conversation

Andarist
Copy link
Contributor

While reading the source code I've noticed that this could produce such text:
Given action an action
which is really unnatural to say. So I've refactored it a little to be more human-friendly and produce either:

  • Given action "SOME_TYPE"
  • or Given an action

Also - if you dont like this String(variable) casting approach I may revert this change and amend my PR. But personally I think its more obvious in what it does (casting) + its shorter by 3 chars :P

@timdorr
Copy link
Member

timdorr commented Mar 20, 2017

We require that all actions have a type field, so I'm actually not sure how that could happen. I would just remove the default text instead.

@Andarist
Copy link
Contributor Author

Yeah, I was kinda wondering why this fallback is there as Ive remember about the check you have mentioned, but thought this could be intended for some reason. Maybe its there because theoretically combineReducers outcome could be used outside of redux? And the check you have mentioned is in the dispatch itself.

At first I thought this test is testing exactly this, but now I see that 'an action' in the test refers to this part.

That being said if you decide I should drop this fallback entirely I think this safe-guard should be dropped, as we would treat then .type to be available always in the reducer.

Let me know what you decide and I'll update my PR accordingly.

@Andarist
Copy link
Contributor Author

Andarist commented Aug 9, 2017

@timdorr ping ;)

this is a silly PR really, but would like to have it stop hanging as open in my PR tab, I understand though if you are too busy right now

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.

2 participants