notskm / vscode-clang-tidy

MIT License
49 stars 25 forks source link

Add fix on save capability #19

Closed ErwanDL closed 4 years ago

ErwanDL commented 4 years ago

This references #9.

I went for the Fix On Save option here rather than the Quick Fixes one, tell me what you think in terms of UX. I am personally more in favor of Fix On Save (it's notably used by the ESLint extension, so I think a lot of VSCode users are used to it), but this is debatable.

I do have 2 concerns though :

What do you think ?

notskm commented 4 years ago

Thank you for the pull request! I'm sorry, I haven't had the time to work on this project like I've wanted too lately.

To address your concerns,

The delay is 100% clang-tidy's fault. It is S L O W. For large files, it can take 30 seconds or more to finish. Even smaller files often take a second or two. There's nothing this extension can really do about that, unfortunately.

Clang-Tidy doesn't give us progress updates, so a status bar is impossible. A notification is probably a good idea, though. It can at least tell the user that they shouldn't touch the file until fixes have been applied.

On top of this, I think there needs to be a command to fix the current file. That way, fix on save can be disabled, but the user can still apply fixes. I'll probably do this myself in a few hours, when I get a chance to test this pull request on my machine.

The boolean flag is fine for now. The whole extension needs to be refactored at some point. The whole thing is quick and dirty like that.

ErwanDL commented 4 years ago

I can try and add a notification while the fixing is happening. By status bar I didn't mean a progress bar, but rather an item that appears in the status bar at the bottom of the VSCode window : the withProgress API of VSCode can help display a spinner and a message while clang-tidy is doing its thing, even if we don't know how much time is left.

Agreed that adding a few tests in the bunch could give better confidence while working and refactoring the code 👍 I also suggest adding at least amax_line_length in the .editorconfig, as I had to disable my Prettier extension because it was trying to format the heck out of every single line in the code.

notskm commented 4 years ago

If you want to take a shot at adding a notification, please go ahead. Otherwise I'll either do it in a few hours or when I've got some extra time.

I'll probably leave the .editorconfig file as is and just start running the code through Prettier after this is merged. I should have just used Prettier from the start.