Skip to content

Conversation

@CodinCat
Copy link
Contributor

@CodinCat CodinCat commented May 3, 2017

@timdorr
Copy link
Member

timdorr commented May 3, 2017

This one is good as-is. Thanks!

@timdorr timdorr merged commit f878baa into reduxjs:master May 3, 2017
@gaearon
Copy link
Contributor

gaearon commented May 3, 2017

Hmm. If we adopt Prettier maybe we can also bring semicolons back 😈 ? I only adopted no-semi style because that's what rackt styleguide said two years ago.

@gaearon
Copy link
Contributor

gaearon commented May 3, 2017

In fact it's been a little annoying to defend the use of no-semi in Redux considered I never liked no-semi in the first place.

@timdorr
Copy link
Member

timdorr commented May 3, 2017

I'd just say it is what it is at this point. The pain in converting to no-semi was bad enough that I don't really want to live through that again. It basically breaks every open PR. And other than preference, it doesn't really net us any tangible benefits (at least not enough to warrant the effort required).

@markerikson
Copy link
Contributor

I'm a four-space/semicolons person myself, but I also hate formatting PRs clogging up file history. So, yes, I'd rather not make changes just for the sake of changes (and once again, considering the relative stability of the lib, don't think it's worth the hassle at this point).

@CodinCat
Copy link
Contributor Author

CodinCat commented May 4, 2017

So.. do we have any conclusion on this? It won't be too difficult to bring semis back at this point since I haven't made any changes to the reviews of PRs, just need to re-run Prettier. But after that it may take more works to add them back.

@gaearon
Copy link
Contributor

gaearon commented May 4, 2017

I don't care—let's leave it as is then.

It basically breaks every open PR.

Easy to fix by running prettier in those PRs but I see your point.
(FWIW we did this with React and it was fine for us.)

@gaearon
Copy link
Contributor

gaearon commented May 4, 2017

Also, since we're running Prettier anyway, we're already breaking every PR, are we not?

@timdorr
Copy link
Member

timdorr commented May 4, 2017

This is only on the code blocks for the docs.

@CodinCat
Copy link
Contributor Author

CodinCat commented May 6, 2017

Personally I prefer no-semi. But this is documentation, maybe with semicolons is less confusing for beginners?

What we need here is just consistency. Some chapters are already in semi style (mostly the StructuringReducers section) so whatever with or without semis, we need to make a lot of changes anyway. But I truly believe it’s worth it since it improves the quality of docs (and fixes some errors).

Hope I can have a confirmation from you guys. 😸

@ccPrathap ccPrathap mentioned this pull request May 11, 2017
@CodinCat
Copy link
Contributor Author

I'm continuing the work. If we really need to bring semis back I can open another PR for that. For now let's just cleanup the docs.

seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
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.

4 participants