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

Use eslint and prettier #71

Merged
merged 4 commits into from
Dec 5, 2017
Merged

Use eslint and prettier #71

merged 4 commits into from
Dec 5, 2017

Conversation

smacker
Copy link
Collaborator

@smacker smacker commented Dec 1, 2017

It will allow to use consistent code style

Copy link
Member

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

Great idea!!!
We (at Platform team) decided to standardize this things across our front projects through https://github.com/src-d/platform-starter
You could maybe take a look :D

package.json Outdated
@@ -25,5 +30,9 @@
"build": "react-scripts build",
"test": "react-scripts test --env=jsdom",
"eject": "react-scripts eject"
},
"prettier": {
Copy link
Member

Choose a reason for hiding this comment

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

Is there no way to avoid this config here? Can not be moved any other place? 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there are 3 options how to save config: https://prettier.io/docs/en/configuration.html
I prefer in package.json. But if you like .rc file, I'll change it

Copy link
Member

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

I don't like to have different solutions to the same problem, but if the consensus on platform-starter can not be followed here is bblfsh/dashboard because of a limitation of the tools used in this project, or due to its particularities, lets go ahead with this solution.

@dpordomingo
Copy link
Member

@smacker you can merge once you had:

  • separated the modifications in two commits:
    • first with the lint configurations and so,
    • the second one with all the modifications automatically made by the linter or required by it
  • passed the @jbeardly requirement about the "Signed-off-by" more info, and here

@dpordomingo dpordomingo closed this Dec 1, 2017
@dpordomingo dpordomingo reopened this Dec 1, 2017
Signed-off-by: smacker <max@smacker.ru>
Signed-off-by: smacker <max@smacker.ru>
Signed-off-by: smacker <max@smacker.ru>
Signed-off-by: smacker <max@smacker.ru>
@smacker
Copy link
Collaborator Author

smacker commented Dec 1, 2017

@dpordomingo rebased. I don't have rights to merge. You need to press a button.

@bzz
Copy link
Contributor

bzz commented Dec 4, 2017

LGTM. and @dpordomingo same lack of merge rights here

@smacker smacker mentioned this pull request Dec 5, 2017
@smacker
Copy link
Collaborator Author

smacker commented Dec 5, 2017

Yeah! I have right to merge now.
I rebased commits with sign off, but CI still says they are missed. It's weird. But looks like all good now.
I'm merging.

@smacker smacker merged commit 2c08aa6 into bblfsh:master Dec 5, 2017
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.

3 participants