-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove function wrapper / add matching dprint and eslint configurations #7596
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@linonetwo ... Can you have a closer look? What do you think? |
From the size comparisons we can see, that removing the wrappers and adding dprinted formatting we end up with the same size as we have now.
|
dprint configuration settings can be found at: https://dprint.dev/plugins/typescript/config/ |
I think you should use dprint integration with eslint, otherwise eslint formatting may conflict with eslint's. And eslint provides a unified autofix alt+shift+G (may need to config hotkey), which autofix eslint problem with dprint formatting problem in VSCode. If you separate them, each code will require 2 hot key to format, which will make it annoying to code in tw.
This is because eslint also autofix many other problems, not only formatting. But this may not be desired. But in the future, I think we should use dprint binding with eslint for dev experience in VSCode. |
This is very helpful. |
I did use the combination with dprint plugins for eslint. The disadvantage is that it needed 2 minutes to do the formatting that dprint alone can do in 2 seconds with absolutely the same result. I did have a big problem with VSCode, because dprint setting and eslint setting did work against each other. With the settings I did add to dprint.json eslint is not needed for prettyprinting. ESLint can be installed as a plugin to VSCode. Dprint can be installed to VSCode and they work together without any fight. But the CLI needs to be dprint alone. It's much faster and does not produce any conflicts. |
This is only the first time. You can do this just using dprint, but we need to add back integration later. And it only takes 0.xS in daily use.
You can copy the config from my PR, which don't have any conflict, acturally I already made a https://github.com/tiddly-gittly/eslint-config-tidgi and https://github.com/tiddly-gittly/TidGi-Desktop/blob/406062b83262440e9ad8708205d6d03af5ff7ec9/.eslintrc.js#L36-L40 without any issue.
Yes you can do this for the first format in this PR. |
As I wrote with your configuration there where conflicts. |
There is not need to add eslint as a dev dependency to TW package.json. It's OK to install the ESLint and the DPrint extensions to VSCode, to get all the info in VSCode and when the code is saved it is prettyprinted and ESLint does it work too. But when TW is created from the command line, dprint is the only dependency we need as this PR shows. |
As long as you don't need those autofix. I use tons of eslint plugin because my time is precious, so I automate as many things as I can. I think tw devs are too, so I tend to introduce those dev tools. But you work more in tw codebase as I did, so it is up to you. I mostly work in TidGi and https://github.com/tiddly-gittly/Modern.TiddlyDev plugin environments now, and I have full set of https://github.com/tiddly-gittly/eslint-config-tidgi on them, so I don't have strong opinion here in tw core codebase.
what is that? We can fix it instead of just drop it |
ESLint autofix are done as soon as you save a code file, if the ESLint extension is installed in VSCode and the configuration setting is activated.
It seems that the dprint-eslint-plugin wanted to add 1 extra line at the end of every code file but eslint had a problem with that. So in the end every save the number of lines at the end changed. I could not fix that with any configuration setting. For some constructions like spaces between commands and braces eg: I think we do not need eslint-dprint-integration on the CLI level. IMO dprint is enough to get a consistent code style, with the dprint.json setting in this PR. |
Is dprint-integration properly installed? I don't know this happened last time, I will take time to check this. But I think you might not have packages properly installed.
This is OK, but we should use it with VSCode. CLI level only need to be done this time in this PR |
Yes it was installed and configured properly. But if eslint rules are disabled anyway, then we do not need them. So we do not need the extension. It does not make sense. eslint-plugin-dprint-integration seems to be no official ESLint plugin. I do not want to invest time into debugging 3rd party extensions and mess with incompatible settings. It's not worth it. Removing the dependency imo will be the best choice in the long run. It will be enough work to keep dprint up to date. |
Hi @pmario I think it might be easier to review this PR if we could remove the whitespace updates, and just include the changes necessary to get dprint installed and running. |
As soon as dprint is active it will destroy the formatting for every tiddler because of the function wrapper
So IMO we cannot test the one without the other. -- I'll add a list of tidders here in the comment section, that contains the changes. Then the rest will be function-wrapper fixes. |
Thanks @pmario it would make the commit history very confusing. Why can't we merge the dprint integration in one PR and then immediately after merge the changes that it makes on first run? Thinking about it, I thought we had decided to remove the function wrappers from modules in any case? In which case that should be done first as a separate PR? |
OK. I'll make this one a draft and create 2 new ones. |
This PR was created to be able to test "dprint" with the TW repo. That's why dprint formatting is not part of the PR.
This PR is related to: Use DPrint to ensure consistent code formatting #7474 ... But there are some differences
dprint.json
configurationeslintrc.json
configurationThis PR removes the function wrapper from all .js modules, where possible.
tiddlywiki edtions/tw.com --build
does create working wikis.After downloading this branch it's possible to use the "dprint fmt" command from the TiddlyWiki5 directory.
So after switching to this branch you can do the following:
It will download the the dprint
typescript
,JSON
andmarkdown
wasm plugins.Then it will "dprint" the whole TW project.
It will modify 525 files and pretty-print them. I did manually compare and review them 1 by 1. It looks good and it does improve code readability
All in all the configurations are very close to the existing TW styleguide. There is 1 major difference. dprint adds spaces between function arguments. There is no setting at the moment, that would allow us to change that.
eg:
is converted to see the space between
params
andcommander
@linonetwo did create an issue at the dprint repo: dprint/dprint-plugin-typescript#526 ... There was no response yet. But it may be configurable in the future.
There is an dprint error message atm, which can be ignored for this PR. But we should create an issue at the dprint repo in the future.
Internal dprint error, that should be reported to dprint repo