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

Typo in rescue for server rendering #963

Merged
merged 4 commits into from
Oct 28, 2017
Merged

Typo in rescue for server rendering #963

merged 4 commits into from
Oct 28, 2017

Conversation

AnatoliiD
Copy link
Contributor

@AnatoliiD AnatoliiD commented Oct 23, 2017

This change is Reviewable

@justin808
Copy link
Member

@railsme this is GREAT!

Can you please add a CHANGELOG.md entry and I'll merge this.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@AnatoliiD
Copy link
Contributor Author

done 👌


Comments from Reviewable

@justin808
Copy link
Member

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


CHANGELOG.md, line 11 at r2 (raw file):

*Please add entries here for your pull requests.*
#### Fixed
- rubocop disabling comments in rescue statements [PR 963](https://github.com/shakacode/react_on_rails/pull/963) by [railsme](https://github.com/railsme).

We can make this description a bit more detailed, along the lines of the rubocop comment broke...and this fixed ...

plus, please rebase your change on master

I can't merge right now due to a conflict...


Comments from Reviewable

@AnatoliiD
Copy link
Contributor Author

Should be better now 😃

@justin808
Copy link
Member

@railsme We're not passing Travis

@AnatoliiD
Copy link
Contributor Author

@justin808 #899 broke the build. Should I try to fix it in my PR or wait for fix from the author? 🤔

@justin808
Copy link
Member

If you can fix it, I’d be thrilled. In this pr is fine.

@AnatoliiD
Copy link
Contributor Author

@justin808, I've found that we are using empty strings as default values for not set config options.
I have doubts about checking directory in LocalesToJs because we have similar checks here.
I suppose that #899 should be reverted or totally replaced in separate PR because this is not part of the current fix.

@AnatoliiD
Copy link
Contributor Author

AnatoliiD commented Oct 28, 2017

@justin808
Please, check and accept #967 😄
I will rebase current branch on top of master.

@justin808
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@justin808 justin808 merged commit 3ce836b into shakacode:master Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants