modelina
modelina copied to clipboard
Add .prettierrc to project
Reason/Context
When working on a custom Go preset, ESLint was complaining about listing rules I was violating. Somehow I couldn't configure VSCode's prettier/formatter integration to stop formatting code when there is no prettierrc.
Overall I think it would help contributors quite a bit to have prettier on the project. Having to manually deal with formatting is a pain and prettier is pretty standard and well supported.
Description
- Add prettier as a dependency
- Add a
.prettierrc
- Update
.eslintrc
to disable rules that are handled by prettier.
Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Normally I would not like the idea of having two competing libraries each working on a specific aspect, it just complicates things in my opinion.
Can you give a concrete example with eslint that your could not get to work in VSCode, and I can see if I can replicate it? 🙂
Does the ESLint config in Modelina usually format as well? My problem was that Prettier would kick in as the default formatter on save because for some reason VSCode wasn't respecting the settings I thought I had set. Then Prettier would pick up some default config to use double quotes and ESLint wouldn't format and complain that I should use single quotes.
I was somewhat under the impression that all of JavaScript/TypeScript has moved on to using ESLint for code style, semantic, syntax issues etc. and Prettier for pure formatting. Maybe my impression was wrong. 😅
Does the ESLint config in Modelina usually format as well? My problem was that Prettier would kick in as the default formatter on save because for some reason VSCode wasn't respecting the settings I thought I had set. Then Prettier would pick up some default config to use double quotes and ESLint wouldn't format and complain that I should use single quotes.
There are a setting in VSCode that enables auto-formatting on save, I always have that off and let eslint handle the formatting ( i.e. you can run npm run lint:fix
to fix most common format issues https://github.com/asyncapi/modelina)/blob/c9980fc8a48dda87bf2e575c23719bbe8fb7ad57/package.json#L81)
But it is not the first time that we have code submitted where it is formatted incorrectly, and if VSCode and other IDÈs use the prettier formatter and it's a standard, then we should probably have that!
Do you have any resources that back it?
Otherwise, we might need to describe this in the development doc.
I was somewhat under the impression that all of JavaScript/TypeScript has moved on to using ESLint for code style, semantic, syntax issues etc. and Prettier for pure formatting. Maybe my impression was wrong. 😅
It might be, I have not heard about it 😄
But it is not the first time that we have code submitted where it is formatted incorrectly, and if VSCode and other IDÈs use the prettier formatter and it's a standard, then we should probably have that!
@jonaslagoni I agree that we will get wrongly formatted code. It is because we haven't set any standards to format the code in our repository. Every IDE or every user has a different perspective and tool to format the code. If we set some standard using .prettierrc
that uses npm run prettier-format
command to format the code in the way written in .prettierrc
.
ESlint is used to fix syntax errors, semantic issues, etc. but does not give formatting the code feature in repository.
Do you have any resources that back it?
Yepp, we can have Prettier
as a DevDependency to enable it. I will better suggest you go through this article for more clearance.
This issue has been automatically marked as stale because it has not had recent activity :sleeping:
It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.
There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.
Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.
Thank you for your patience :heart:
We definitely need this 😄
@jonaslagoni Look over to this comment https://github.com/asyncapi/modelina/issues/646#issuecomment-1046275156.
Instead of having husky pre-commit, we can have a GitHub action to run a particular script whenever there is a commit or push to PR or master branch. I think this can be a good approach instead of using husky. What's your view on this?
I think husky might be the right choice.
The problem with using GH Action is that it does not happen directly and you need to wait for the action to commit the formatting changes, which I don't think is the best user experience 😅 Is there something about husky you think would make it a bad fit here @akshatnema ?
I made the same issue on the website also to fix the lint issues and code formatting using husky. But eventually, it got closed because many community members aren't in the favor of using husky recommit. You can read the discussion on the issue https://github.com/asyncapi/website/issues/598. That's why I suggested having a Github action.
Thanks for the link @akshatnema, that's definitely some good perspectives 🤔
Okay so for the bare minimum we need to add prettier, that is step 1, and is needed regardless 😄 So adding that ensures that whenever PRs are merged, they are always correctly formatted no matter how a PR change looks like. So I think we should focus on that first, what do you say @akshatnema?🙂
Want to champion this @akshatnema ?
So adding that ensures that whenever PRs are merged, they are always correctly formatted no matter how a PR change looks like.
So you want that on every push insider a PR, you want a prettier script run before merging the PR, right?
Want to champion this @akshatnema ?
Fine, I can add this. First, let's ask the community members (available right now) what are their reviews on how the script should be run in the repo. Asking for reviews from @derberg @fmvilas...
Yes 😄 Initially, we just add a prettier configuration and then tie it together with the linter as you linked https://khalilstemmler.com/blogs/tooling/prettier/#Configuring-Prettier-to-work-with-ESLint. That way we don't have to adapt any GitHub workflows to enforce it.
This means that PRs will fail in the linting run if there are any prettier warnings. Which the developer would then have to manually fix by running npm run format
(or similar). It's the least invasive approach that we can start with.
Also no need to ask for permission from Lukasz and Fran as they are not codeowners of this repo 😄 But of course, anyone is welcome to voice their opinion 💪
cc codeowners @magicmatatjahu @Samridhi-98 @ron-debajyoti in case you have some veto against it 🙂
This issue has been automatically marked as stale because it has not had recent activity :sleeping:
It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.
There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.
Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.
Thank you for your patience :heart:
Already solved