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 prettier to detox folder #278

Merged
merged 1 commit into from
Nov 29, 2017

Conversation

AlanFoster
Copy link
Contributor

This is the first PR to help deliver #223

This Pr adds support for prettier as part of detox's linting pipeline. In the future you can automagically fix errors with:

npm run lint -- --fix

In a separate PR I will add support for connecting this up to the CI pipeline, and husky for pre-push validations 👍

@AlanFoster
Copy link
Contributor Author

cc @DanielMSchmidt / @rotemmiz

@DanielMSchmidt
Copy link
Contributor

Could you maybe add a config to set spaces to tabs? That would be awesome!

@isnifer
Copy link
Contributor

isnifer commented Sep 15, 2017

@AlanFoster Nice work. Awesome!

@DanielMSchmidt
Copy link
Contributor

And having the prettierrc on root level would be pretty awesome 👍

@rotemmiz
Copy link
Member

rotemmiz commented Sep 16, 2017

There are two big PRs adjacent to this PR (#268, #277) , please let's make them land in master before this PR, since there may be some ugly conflicts when merging.

If you can, it will be best if you could split the configuration of prettier from the actual code changes its linter generated, that way we could merge the config right, in and won't need to wait.

@AlanFoster
Copy link
Contributor Author

@rotemmiz Sounds good to me 👍

As a quick question - would you prefer tabs or spaces? Or should we just stick to the default settings?

@DanielMSchmidt
Copy link
Contributor

Tabs please, as discussed in #223. For the rest I would stick with the defaults :)

@AlanFoster AlanFoster force-pushed the add-prettier-to-detox-folder branch 3 times, most recently from 4c2b89c to b034008 Compare September 18, 2017 17:52
@AlanFoster
Copy link
Contributor Author

AlanFoster commented Sep 18, 2017

@rotemmiz / @DanielMSchmidt I've dropped the commit that actually does the prettier transform. For now this is a configuration only PR as requested.

I have also dropped any rules that directly conflict with prettier, and I have decided to not progress with a .prettier file - in favour of keeping it within the eslintrc file:

    "prettier/prettier": [
      "error", {
        "useTabs": true
      }
    ],

Once the blocking PRs are merged in, I can put up another PR that has run npm run lint -- --fix and send future PRs which update the CI pipeline etc 👍

Let me know if there are any other changes to be made :)

@rotemmiz
Copy link
Member

Hey @AlanFoster , can you please update the branch and resolve conflicts, I want to merge this PR after CI is green.

Thanks!

@rotemmiz
Copy link
Member

Hey @AlanFoster , can you please update the branch and resolve conflicts, I want to merge this PR after CI is green.

Thanks!

@AlanFoster
Copy link
Contributor Author

@rotemmiz Thanks for the poke; Rebased as requested 👍

I can put up further PRs to fix the linting issues, or you can run npm run lint -- --fix directly yourself too, just let me know what you'd want like :)

@DanielMSchmidt
Copy link
Contributor

@AlanFoster Can you take a look at #432? I redid the changes you introduced, this PR is somehow not rebaseable...

@AlanFoster
Copy link
Contributor Author

@DanielMSchmidt For visibility the this PR is rebased and should be good to go still :)

@DanielMSchmidt
Copy link
Contributor

Awersome, thanks @AlanFoster 👍

@rotemmiz rotemmiz merged commit 9d7e008 into wix:master Nov 29, 2017
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants