Endjin.RecommendedPractices.NuGet v3 possible features

[idg10]

There are a few changes I'd like to make that are breaking changes, including:

• requiring the new(ish) non-nested namespace syntax
• enabling style-related diagnostics to be ignored in Debug configuration (so you don't have to fix them in your inner dev loops, but will still be required to fix them all before committing, because they'll still be considered errors in the CI build)
• detect when your .editorconfig is out of date and offer to update, but in a way that deals gracefully with project-specific custom changes (e.g., don't just blindly update the thing every time)

[mwadams]

Is there some way to require a release build before push using git actions?

[mwadams]

The situation I want to avoid is push=>carry on working=>eventually get a notification (30 mins later) that I wasn't in fact done=>try to unwind

[idg10]

Does the fact that all the relevant diagnostics are visible in the Error List tab not help with this? You can find out immediately whether you're done by looking at that.

The only difference with the current approach is that for the random subset of diagnostics that are warnings not messages, it will get in your face by failing to build, so you find out you've done this even if you don't have that tab open.

[mwadams]

So we could do something like you suggest where a Debug build adds a marker file to the solution and a release build removes it.

A server-side git pre-receive hook could check for the presence of the file (or absence if we want to reverse the policy) and abort if it is in the unwanted state.

This would have the advantage of being able to force it by manually deleting the file.

[idg10]

Can you install server-side git pre-receive hooks in GitHub though?

[idg10]

TIL how to write Git hooks in PowerShell. If you create a .git/hooks/pre-commit file with this content:

#!/bin/sh
pwsh -f `git rev-parse --git-dir`/hooks/pre-commit.ps1

and you also create a .git/hooks/pre-commit.ps1 file, that script will be executed each time a commit is attempted. (The significant of pre- is that if someone just types git commit without a message, this script runs before git fires up an editor for your commit message. So this means if the hook wants to block the commit it can do so before the user has put effort into writing a message for a commit that is doomed.)

The script can block the commit with a non-zero exit code. So we could write this sort of thing:

Write-Host "Powershell FTW!"
$RepoRoot = git rev-parse --show-toplevel
Write-Host $RepoRoot
$BuildResultPath = Join-Path $RepoRoot .endjintemp/LastBuild.json
$BuildResultContent = Get-Content -ErrorAction SilentlyContinue -Path $BuildResultPath
if (-not $BuildResultContent)
{
    Write-Host "There is no record of the last build"
    Write-Host "Either you have not yet built this project, or you are not using Endjin.RecommendedPractices.NuGet (in which case its odd that you have this commit hook installed)"
    Write-Host "There should be a file at $BuildResultPath"
    Exit 1
}
$LastBuildResults = $BuildResultContent | ConvertFrom-Json
$LastBuildConfiguration = $LastBuildResults.$LastBuildConfiguration
Write-Host "Last build configuration was $LastBuildConfiguration"
if ($LastBuildConfiguration -eq "Release") { Exit 0 }
Exit 1

[HowardvanRooijen]

I'm not a huge fan of relaxing linting rules in debug mode because it may improve your inner dev loop for doing technical spikes, but it also allows people to get out of the good habits and then makes the inner dev loop harder because the outer dev loop catches the issues. I tend to find it easier to add pragmas / exclusions if spiking and then remove them as you iterate towards something production ready.

[idg10]

Response summary (TLDR):

1. my proposal is not just (or even mainly) about inner loop efficiency: the code quality and safety aspects are more important
2. my proposal would not require you to change your practices
3. the status quo already encourages bad habits, and in my view there would be a net improvement on this front with my proposal, because the current system forces you to use bad habits that are worse than the bad habits newly enabled (but not mandated) by the proposal

Detail and supporting arguments:

Firstly, a feature of the proposed change is that it won't stop you from adding pragmas / exclusions if that's how you want to work. I just want to enable a safer way of working for those who want that (e.g., me).

The problem with the status quo is that it makes it more likely that we will accidentally bypass these rules precisely because it compels us to "add pragmas / exclusions".

So this isn't just about efficiency in the the inner loop. There is a win there, but it's the less important of the two benefits I originally stated. I would still be strongly in favour of this changer even if there were no inner loop gains.

I'm not a huge fan of relaxing linting rules in debug mode because it may improve your inner dev loop for doing technical spikes

If this were the only benefit, this would be a stronger point (although I disagree with its suggestion that this is just about technical spikes). But it's not the only benefit.

I mentioned two aspects in the original post: the inner loop and whether or not you are "required to fix them all before committing". So this proposal has a positive impact on the average quality of code at the point where it gets merged. The current design makes it very easy to commit your bypass measures instead of fixing them. In fact, it makes that the default. My proposal enables you to work in a way where you will be unable to merge a PR without fixing all these problem. It's one less thing where we're totally reliant on the vigilance of individuals doing code review.

The dangerous aspect of using exclusions and pragmas to fix this is that it normalises the use of a persistent rule bypass to deal with a transient problem. This is a dangerous habit to get into—so if your goal is to reinforce "good habits" then the status quo has exactly the opposite effect.

It also creates an additional problem. Persistently bypassing a rule is sometimes necessary for technical purposes—sometimes those pragmas or exclusions are necessary, but once you start using the same mechanism transiently, it creates new, avoidable work: anyone looking at such an exclusion now has to wonder if it was left in deliberately.

Multiple times a day I end up having to bypass these rules in order to get things done (just like you do). Because the rules present as errors, I have to take steps to make them go away, steps that have a persistent effect. For example, whenever I want to temporarily comment out some code (and I must have done that about 10 times yesterday just in the spike I'm working on with @mwadams as we needed to temporarily disable things to understand the cause of a perf change) I have to double-comment it with //// in order to bypass the rules that insist your comments are formatted in a certain way.

The inevitable result of this is that I sometimes forget to undo this. This only happens because I am forced to make a change to the source code to disable the diagnostic.

Any time you disable a safety guard using a persistent change to effect what is meant to be a temporary bypass, there's now a risk that the safety guard will remain disabled. These pragmas / exclusions make them vanish entirely, enabling the problems to remain unnoticed, creating the possibility that they will be committed accidentally.

But if these things to continue to produce diagnostics the entire time they are present, that removes the possibility of accidental permanence. (Even if you fail to notice they're still there when you commit, your PR's not going to merge because the CI build will fail.)

I tend to find it easier to add pragmas / exclusions

So you habitually remove the safety guards as you work. We are both forced into this same dangerous practice.

We have effectively created a workshop environment in which we force people to remove the safety guards from the power tools.

And just to re-iterate a key point, absolutely nothing about my suggestion in any way stops you from operating in the way in which you are accustomed to. I'm not proposing making the messages disappear. I just want them to be at a level that won't prevent you from running code in its existing state. They will all be right there in your Error List window, and showing up as squigglies in your code. If you want to continue to disable them with pragmas or other exclusions, you can, and they will duly vanish from the diagnostic list like they do today.

The main different is that today, we are compelled to work in that way. This change would enable anyone who wants to work in that way to continue to do so, so it's not stopping you from working your preferred way, but it would enable those who would prefer to allow a diagnostic message to just sit there for a few minutes, and for it to stay there as a reminder until such time as they're ready to address it properly.

This makes the diagnostics lists a more powerful tool: it keeps track of a bunch of things you're not yet dealing with. It's a reminder of the outstanding issues you'll need to attend to before you can push.

It already works that way for quite a lot of diagnostics anyway. There's a rather arbitrary split between things that show up as messages and things that show up as warnings. For no particularly good reason, the ones that raise objections to the most superficial aspects of the code are the ones that block your progress, while a lot of arguably more important technical diagnostics show up as mere messages.

it also allows people to get out of the good habits

Not for long. Only until they push to a PR branch. During normal development you're going to be pushing pretty regularly anyway, so you will learn pretty quickly that you can't just ignore the warnings. Conversely, the status quo enables you to adopt the bad habit of suppressing a diagnostic and then never coming back to it.

[idg10]

@mwadams had this suggestion for ensuring we pick up updates to the .editorconfig:

I guess it would be possible to put a marker in the file with the package version in it, and overwrite the file if it has the old marker.

(and maybe emit a warning as a extra aide-memoire)

Maybe even two markers! The second marker continues to emit the warning until you remove it.

[mwadams]

And I would have said "an extra" if I had read it back.