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

eslint fix #21

Closed

Conversation

shubhamku044
Copy link

I have fixed #18
#11

@vercel
Copy link

vercel bot commented Sep 9, 2023

@shubhamku044 is attempting to deploy a commit to the 100devs Team on Vercel.

A member of the Team first needs to authorize it.

@reviewpad reviewpad bot added the large Pull Requests with lots of added lines label Sep 9, 2023
@vercel
Copy link

vercel bot commented Sep 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
100editors ❌ Failed (Inspect) Sep 9, 2023 7:43pm

Copy link
Contributor

@alcpereira alcpereira left a comment

Choose a reason for hiding this comment

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

I think it's too many rules at the same time making it hard to review, even more with the JSON format (can't add inline comments, see first point below).

What I suggest:

  • Adding an inline comment to prettier in the config to remember that the config needs to be the last one in the list as recommended by their docs. This will require changing the .json format to another one
  • Using directly eslint: recommended and incrementally change what we like or dislike about it.

"prettier"
],
"rules": {
"semi": "error",
Copy link
Contributor

@alcpereira alcpereira Sep 9, 2023

Choose a reason for hiding this comment

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

This (and some other rules below) should be handled automatically by Prettier and overwritten by the prettier config.

Copy link
Contributor

@alcpereira alcpereira Sep 9, 2023

Choose a reason for hiding this comment

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

We are currently using npm for now (there is a package-lock.json in the repo).
Note that because our Workflow is using npm, this won't install the dependency because the package-lock.json was not in sync with package.json. With yarn it will sync yarn.lock with package.json but will left untouched the package-lock.json which is always used by npm ci command.

I don't mind switching the package manager, but this should be done in another Issue/PR and include an update for our workflow.

@reviewpad reviewpad bot added the git-conflict Pull Requests with a git conflict between the head and base branches label Sep 10, 2023
@nmpereira nmpereira closed this Sep 27, 2023
@reviewpad reviewpad bot removed large Pull Requests with lots of added lines git-conflict Pull Requests with a git conflict between the head and base branches labels Sep 27, 2023
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.

Add prettier config to ESlint
3 participants