-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
Signed-off-by: Peter Haumer <4391934+phaumer@users.noreply.github.com>
Signed-off-by: Peter Haumer <4391934+phaumer@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing! 👍
I understand that there may be changes in the future based on what the team feels comfortable with.
But so far, I'm liking it! 😋
Hi @phaumer , this is great! I have several comments. |
The standard way to configure prettier is to use a config file for it in the repository. I would suggest prettier.config.js since the |
Also having eslint run prettier and having both eslint and prettier extension can potentially cause conflicts in what tool gets to report an error. The prettier extension for vscode explicitly states that it should not be used in case prettier is run through a linter. |
My last comment goes to the prettier configuration as present. I understand everyone has their preference. The goal of prettier was to remove those debates and ideally have no settings. I try to stay true to that with as little configuration as possible. This is what I use in most of my projects: module.exports = {
trailingComma: "es5",
}; the reason for the |
Well, I have one more :-) The introduction of prettier into an active codbase has the side effect that everyone has to reformat their branches. This mostly works fine, but there might be some extra work with merging conflicts. So it is a good idea to do it at a point in time when there are as few PRs open as possible. |
Agree with doing it in the Zowe Explorer when there are a low number of PRs open 😋 Interesting that you mention the |
Thanks for the detailed review @VitGottwald. I actually think it is a better option to apply prettier through the linter as we cannot guarantee that all developers will install the VS Code extension. Therefore, I would like to see as much as possible being handled by things provided as devDependencies and not a separate tool people might not want to install. The other advantage I see is that everything is in one configuration file and not two, because prettier is quite an opinionated formatter and does things that are more linting than formatting such as your comma example or adding/removing parenthesis Lambda expression etc. Trailing comma I have not problem changing. For me it is more an OCD thing and it looks just wrong or that the user has forgotten an element ;-) |
I kind of disagree with the running of prettier only in eslint approach. I think it is good to run prettier in CI to make sure nothing was forgotten. This can be done directly or through a linter. One can look at prettier as a linting tool. But in my opinion that misses the point. The real value of prettier is that it does the formatting for us! Why would anyone want to be pacified by formatting errors they have to fix manually? So I do not understand the question about not having prettier installed. We add it to dev dependencies and everyone has it installed. And they get free automatic formatting of the code. Its quite late here so I may have not explained myself well. Will be happy to chat with you tomorrow! |
Thanks @VitGottwald. I think I might still not understand your point. Would you have the time adding a commit here with the changes you have in mind? Is it just extracting the prettier rules into a prettierrc file? |
Signed-off-by: Peter Haumer 4391934+phaumer@users.noreply.github.com
This switches from tslint to eslint and integrates with Prettier to apply formatting on save. Installed the ESlint and Prettier VS Code extensions to see it work in VS Code while editing.
If that approach works for everyone then we could try this on the main repo as well.