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

Deps: add prettier config & adapt eslint config #497

Merged
merged 8 commits into from
Sep 10, 2023

Conversation

Yedidyar
Copy link
Contributor

@Yedidyar Yedidyar commented Sep 1, 2023

what is the motivation:

https://www.youtube.com/watch?v=Cd-gBxzcsdA

if this or accepted we should add format to git hooks

add run yarn format on all project

@baruchiro
Copy link
Collaborator

Thank you!

Even before it is accepted, I'm expecting you to test it is not conflicted with ESLint.

Did you run it and run yarn lint afterwards?

Also, I can't see how it helps. No one will change the whole source code.

Instead, I'm asking you to add it with lint-staged to incrementally format every file that is touched by someone.

https://bscstudent.netlify.app/ci-checklist/

@Yedidyar Yedidyar changed the title Add Prettier Config & Adapt Eslint Config deps: add prettier config & adapt eslint config Sep 1, 2023
@Yedidyar Yedidyar changed the title deps: add prettier config & adapt eslint config Deps: add prettier config & adapt eslint config Sep 1, 2023
@Yedidyar
Copy link
Contributor Author

Yedidyar commented Sep 1, 2023

Did you run it and run yarn lint afterwards?

yes

Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

Very good work!

I will test it locally and then I will merge it.

What will be your next task? Contact me

@Yedidyar
Copy link
Contributor Author

Yedidyar commented Sep 1, 2023

Very good work!

I will test it locally and then I will merge it.

What will be your next task? Contact me

I see that the ci dont pass because of the node version what should we do?

@baruchiro
Copy link
Collaborator

@Yedidyar we have a task for upgrading the nodeJS version: #464

But it is not a quick task.

You should check the lint-staged and find its version that matches the nodeJS v14.

@Yedidyar
Copy link
Contributor Author

Yedidyar commented Sep 3, 2023

@Yedidyar we have a task for upgrading the nodeJS version: #464

But it is not a quick task.

You should check the lint-staged and find its version that matches the nodeJS v14.

cf4e936

Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

There is still a mismatch between prettier and ESLint.
When I'm running yarn prettier --write . and then yarn lint, I see multiple warnings like this:

http://eslint.org/docs/rules/object-curly-newline Expected a line break before this closing brace src\Store.test.tsx:3:56

I'm navigating to this file (src/Store.test.tsx:3:56), formatting it with prettier, and it shows me an ESLint worning. I'm fixing the ESLint warning, and formatting it again, and the warning cames back.

@Yedidyar Yedidyar requested a review from baruchiro September 7, 2023 02:13
@Yedidyar
Copy link
Contributor Author

Yedidyar commented Sep 7, 2023

@baruchiro 26c4866 see this commit a think it the right approach

@Yedidyar
Copy link
Contributor Author

Yedidyar commented Sep 7, 2023

Very good work!

I will test it locally and then I will merge it.

What will be your next task? Contact me

I think the next step is to move to using Vite.
what is the specific current version of node?

@baruchiro baruchiro merged commit 6896315 into brafdlog:master Sep 10, 2023
github-actions bot pushed a commit that referenced this pull request Sep 10, 2023
## [1.13.1](v1.13.0...v1.13.1) (2023-09-10)

### Deps

* add prettier config & adapt eslint config (#497) ([6896315](6896315)), closes [#497](#497)
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.13.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants