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 ESLint and Prettier configuration #14

Merged
merged 3 commits into from
Oct 31, 2022
Merged

Add ESLint and Prettier configuration #14

merged 3 commits into from
Oct 31, 2022

Conversation

lucaslencinas
Copy link
Contributor

@lucaslencinas lucaslencinas commented Oct 25, 2022

Adding some basic configuration for having ESLint.
Nothing too fancy, just basic configuration

Also, adding configuration for Prettier.
More info on their page https://prettier.io/docs/en/index.html
Make sure you set up Prettier on your editor so you can have it executed everytime you save a file automatically and modifying

@@ -26,5 +28,8 @@
"devDependencies": {
"@lerna/clean": "^5.5.2",
"lerna": "^5.5.2"
},
"resolutions": {
"typescript": "~4.7"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to fix the typescript version due to a bug on parcel
parcel-bundler/parcel#8419

@lucaslencinas lucaslencinas changed the title Add eslint configuration Add ESLint and Prettier configuration Oct 25, 2022
cmd: lint:test # will run `yarn lint:test` command
- uses: borales/actions-yarn@v3.0.0
with:
cmd: prettier:check # will run `yarn prettier:check` command
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't checked this much. I want to run this command while building a PR and also when merging to main

Copy link
Contributor

Choose a reason for hiding this comment

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

wait why do we need both? can't we just use lint:test or prettier:check? if we need both I'd suggest we wrap it in our package json

* Being respectful of differing opinions, viewpoints, and experiences
* Giving and gracefully accepting constructive feedback
* Accepting responsibility and apologizing to those affected by our mistakes,
- Demonstrating empathy and kindness toward other people
Copy link
Contributor

Choose a reason for hiding this comment

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

just a question: was this because of the linter too or is it unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because of the linter. Looks the same on github preview mode so I accepted those rules on the linter. If we find problems later, we can adapt and tweak it a bit.

README.md Outdated
@@ -12,20 +12,27 @@ For now, there are very few commands and will change soon.
yarn install ## or npm install
```

- Build the SDK library
- Build the SDK library and the React frontend
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest

and the React sample frontend

So that we are crystal clear that the main component is the SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

@arielsegura
Copy link
Contributor

LGTM, I left a few minor comments but when you fix the conflicts I can approve

@lucaslencinas lucaslencinas merged commit f9410cc into main Oct 31, 2022
@lucaslencinas lucaslencinas deleted the add-eslint branch October 31, 2022 20:24
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