-
-
Notifications
You must be signed in to change notification settings - Fork 632
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 and Eslint Checks as GitHub PR Merge Workflows #1640
Comments
@derberg @mcturco @akshatnema @magicmatatjahu Please take a look at this issue. This is a crucial change and would help in automating the lint and formatting of the code base. |
@derberg @mcturco @akshatnema @magicmatatjahu If possible I would love to work on this issue . |
…yncapi#1640 Signed-off-by: SumantxD <[email protected]>
@anshgoyalevil I have tried to fix the issue here is the PR #1666 |
Signed-off-by: SumantxD <[email protected]>
@SumantxD @anshgoyalevil IMHO, I will not prefer to have Prettier configured in the codebase as a part of script. Yes, I agree that we sometimes come across the cases where contributor uses the Prettier or any code formatter to format the code in any style. But I prefers to have no code-formatter configured inside the code and everyone should follow the normal Javascript and Typescript formatter of the VS code. Regarding the eslint configuration, yeah please add it to the scripts inside package.json but don't configure it as a part of any hook or workflow for the repository. It's nice to be implemented in the way if someone wants to run the eslint at their own wish. These are just my opinions, no final calls at all. I will recommend to take views from @derberg and @magicmatatjahu also regarding this. And @SumantxD, please make sure that you should first see whether the issue has been acknowledged by one of the maintainers of the repository before creating PR for the issue. |
Thanks for the views regarding Prettier Configuration. I too agree with those points. Some open-source projects are using it as a mandate, so I am a bit confused if it is necessary.
According to me, the reason for adding eslint to workflow was to enforce the contributor to follow some standard coding rules, which not only maintains the integrity of the project, but also ensures that code is readable. |
I think there are 2 different topic: eslint and prettier. Prettier - no there is just too much of it, like additional formats that mess with PRs, @anshgoyalevil made a good point in #1647 (comment) that it is about time to think on a single package where we have config for all. I remember some time @magicmatatjahu mentioned we need it but nobody had time to take care of it. thoughts? |
I made the plugin according to the issue asyncapi/community#238 with the following extensions: plugin:@typescript-eslint
plugin:sonarjs
plugin:security
plugin:react
plugin:jest If that's appropriate, we can take it as a base and add more extensions according to more specific use case. Package Link: https://www.npmjs.com/package/@anshgoyalevil/eslint-config |
before we move forward, let's see it in action @akshatnema @magicmatatjahu so what about using it? |
@anshgoyalevil can you please open a new fresh issue for discussion about using https://www.npmjs.com/package/@anshgoyalevil/eslint-config in website? and |
Sure @derberg, will create an issue regarding that. Since, the addition of |
why? what effects can prettier have on eslint? |
@derberg oh yes, just gave it a thought. No effect 😄 |
Reason/Context
Please try answering few of those questions
Why we need this improvement?
Currently, we do not have prettier under this project. Due to which, the codebase is improperly formatted.
How will this change help?
This change would allow us to manage the lint and format changes during PRs.
What is the motivation?
Most of the open-source projects I have seen or contributed so far uses these plugins and further have a GitHub action to check for such error during the PR workflow.
Description
Please try answering few of those questions
The following scripts needs to be included:
Further, a GitHub action needs to be added to check the PR files, if they fail or pass in the lint and format check.
Will this be a breaking change?
Yes
How could it be implemented/designed?
This could be implemented by adding the above npm scripts, and .yml files.
The above codes are for representation purpose only. Actual code may vary while implementing.
The text was updated successfully, but these errors were encountered: