al-code-outline icon indicating copy to clipboard operation
al-code-outline copied to clipboard

Add pre-commit support for cleanup actions

Open cegekaJG opened this issue 1 year ago • 5 comments

The different cleanup-commands such as "Remove unused variables" and "Fix casing" would benefit greatly if they were viable pre-commit hooks. AL desperately needs hooks that ensure proper coding before pushing a commit.

cegekaJG avatar Jul 18 '23 07:07 cegekaJG

Adding link as a reference (as some concerns were raised here) https://github.com/anzwdev/al-code-outline/issues/295

fvet avatar Sep 06 '23 15:09 fvet

Thank you @fvet for linking that other discussion.

As I mentioned there, I am not sure if running code transformation at commit stage is a good idea, because then the code that is sent to the repository is not the same that you used for your local tests. I feel more secure if some "light" transformations are applied automatically during document save and all other rules are enforced by running code analyzers by devops from pull requests to protected branches.

"Remove unused variables" is an interesting example. It can save a lot of time, but running it without code review is a risky idea. The warning that variable is not used my be generated because of 2 reasons - variable might really be not used, but there is also a possibility that it is a bug - parameter/global variable might be used instead of a local one.

That's why I am not sure how far we should go with creation of other automatic code transformation commands/tools. Some code analyzer warnings are there to tell you "there might be a bug in your code, it would be good to review that line".

anzwdev avatar Sep 23 '23 18:09 anzwdev

I'd like to reopen this subject as a lack of standardized cleanup procedures has become more and more of an issue for our team. First of all, it's important to establish that pre-commit hooks don't stage and commit files, and they don't have to modify them either. By default, a hook will check whether all staged files fulfill a certain criteria, like whether json properties are sorted. If at least one doesn't, the entire hook will be marked as a failure, and depending on the hook, the bad files are corrected, without staging the new changes. This ensures two things:

  1. It's possible to automate read-only checks and use them in pipelines, like a status check for pull requests.
  2. Any modifications from the hook can be viewed separately from all other changes and reviewed accordingly.

For starters, I recommend adapting individual cleanup actions as a pre-commit hook, as well as something that doesn't require looking across multiple files - sorting variables, for example. The biggest hurdle is that these hooks run outside of VS Code and shouldn't have any dependencies to it. We can assume that the user is using VS Code to edit the project, but a workflow would be working off a clean environment. If you have an idea on how to get around that, I'm happy to help, but that's probably a bigger issue than automated code transformation.

cegekaJG avatar Feb 01 '24 09:02 cegekaJG

Can I get some feedback? If you have an questions regarding pre-commits, I'll answer them to the best of my abilities.

cegekaJG avatar Feb 21 '24 16:02 cegekaJG

Hi

I have a plan to create a command line app that will run code transformations. Once it is ready, you will be able to run it locally or from devops.

anzwdev avatar Feb 21 '24 23:02 anzwdev