MNT Add linting

[GuySartorelli]

NOTES

• Some of the changes required to pass linting PHP code has involved things like adding namespaces, which might affect the context of documentation surrounding that block (e.g. yaml configuration may reference the classname, and needs the full namespace). I've done my best to catch all of those scenarios, but please make sure you're looking for those sorts of scenarios during review.
• There were inconsistencies in how we declared the filename for a code block, and the most common way failed linting. I've unified those to now be included as a comment in the first line of the codeblock itself. I've made an edit to include this as one of our documentation standards going forward.
• I have explicitly disabled the rule which limits the number of characters per line as it simply touches too much documentation to do in this pass. This will be hard enough to merge-up as it is. After this has been merged in and up, I'll create a separate PR that only applies that rule.

DO NOT SQUASH COMMITS

Issue

• Add linting for docs #11

[GuySartorelli]

The individual scripts are used by CI so we can clearly see what is failing and where, without having either a massive hard-to-parse output or a failure in one script which results in skipping the rest of the linting.

The main lint script is useful to run locally to double check all is well before pushing changes.

The lint-fix script is useful to run locally to fix anything that can be autofixed, without really caring what script is fixing it.

[emteknetnz]

In the files change tab at the bottom there a "Unchanged files with check annotations" - any idea if that's something that can be configured off at the github actions level, or if it's a user preference thing?

[emteknetnz]

I'm thinking we should split off the vale language linting into its own card and tackle separately from the non-language stuff like indentation first.

Once that's sorted then look at doing language second because at that point we'll get a mountain of warnings tell us that we should consider replacing words like "simple" and "could"

[GuySartorelli]

In the files change tab at the bottom there a "Unchanged files with check annotations" - any idea if that's something that can be configured off at the github actions level, or if it's a user preference thing?

Looks like that's just something GitHub is doing, which is a pain. There's (a very small amount of) discussion about it at actions/toolkit#457

I think the only reason we don't see this on any PRs in our other repos is because we don't have any pre-existing linting issues in those other repos.

Once we've fixed them all in this PR, we won't see them anymore here either.

[GuySartorelli]

I'm thinking we should split off the vale language linting into its own card and tackle separately from the non-language stuff like indentation first.

After discussing this offline I agree that it will be easier and faster to review the structural linting first, and deal with the language stuff separately as that could change the meanings of what's written and therefore requires a higher level of scrutiny.

See #390

[emteknetnz]

Can we exclude old changelogs (but not new ones, maybe do something with git branch -r)? There are A LOT of linting issues in old changelogs and to me it seems wrong to update old changelogs since they're a snapshot in time, particularly once we add in the language linting.

[GuySartorelli]

Discussed offline - the markdown linting especially should probably be done on changelogs. A lot of the justification for the markdown linting rules is to remove ambiguity that can result in differences between markdown renderers - so if we decide to change what renderer we use to convert the md to html, we won't have unexpected problems rendering these old docs.

We can consider whether the language linting should affect historic changelogs when we deal with the separate issue.

@maxime-rainville can you please just let us know if you're okay with us changing the old changelogs as they pertain to markdown, js, and php linting?

[maxime-rainville]

@GuySartorelli I'm absolutely fine with updating old change log. I don't think of them as an "historical record". If something happens later on that alters the context of the changelog, I have no problem with pushing updates to them.

[GuySartorelli]

@emteknetnz Looks like you didn't look at this one?

[emteknetnz]

My main things

• For file names for code fences, they should be bold above code fence, not in it the code fence
• Remove empty namespaces from php files
• There's a fair number capitilisation problems, this could get really annoying if failing builds because of it and cannot merge because the linter has got it wrong

[emteknetnz]

These empty namespaces are pointless, can we simply not have them if there isn't one?

[GuySartorelli]

They aren't pointless. They're required to pass linting, and they're also reflective of how the Page and PageController classes are declared in the recipe.

[emteknetnz]

Yeah, lots of people in bespoke say they just delete the redundant namespace since it indents everything. We should update those files in the recipe

They're required to pass linting

Then seems like we should make an exception in linting rather than enforcing an annoying standard.

[GuySartorelli]

I don't think it's an annoying standard? It matches what our recipes have, and it is best practice to leave these in. If bespoke choose to remove them, that's up to them. I argue that it isn't redundant, it's explicit - and being explicit is good.

Basically, I think that if we're going to add an exception here, then we should also remove the namespace block from the recipe - and I don't think we should remove the namespace block from the recipe.

[maxime-rainville]

Honestly, I've been ignoring those namespace statement for the last 5+ years and never bothered to check what their purpose was.

Are they there to get around the fact PSR-4 would want Page to be in a namespace, but Page needs to be in the root namespace?

[GuySartorelli]

I don't know what they're there for in the recipe - but they're there. If we're removing them we should remove them from the recipe. If we're not removing them from the recipe we should keep them in the docs.

Currently they're in the recipe, so it makes sense to put them in the docs in this PR. Removing them should be a separate card, if that's what we decide we want to do.

[emteknetnz]

I'd say delete from recipe

[maxime-rainville]

I tried to track down when we added those namespace statement. That's what I found silverstripe/recipe-cms#11 (comment)

Doesn't look like there was much thought that went into this.

We also have this old RFC which doesn't look like it ever got implemented. silverstripe/silverstripe-framework#5844

In the interest of moving this along, I suggest we leave the namespace statement in the doc. For now, if someone cares deeply about removing the namespace statement, raise a separate card.

[emteknetnz]

These two look wrong again

[GuySartorelli]

Fixed

[GuySartorelli]

For file names for code fences, they should be bold above code fence, not in it the code fence

I'll leave discussion about this in #387 (comment)

Remove empty namespaces from php files

Do you mean there are some namespace; somewhere? I can't find any? Please specify.

There's a fair number capitilisation problems, this could get really annoying if failing builds because of it and cannot merge because the linter has got it wrong

I'm assuming you pointed those out in the review. If you didn't, please do so or I won't know what to fix.

[GuySartorelli]

Squashed all commits that update the documentation, because I accidentally adding linting changes to one of the non-linting doc change commits.

[emteknetnz]

Remove empty namespaces from php files

Do you mean there are some namespace; somewhere? I can't find any? Please specify.

I mean the instances where it has namespace { and then indents everything. It's pointless. The Page and PageController in the recipes have this, though they shouldn't. People often just delete these because they do nothing. Probably requires some custom linting rule to require a namespace, unless it's empty/root

There's a fair number capitilisation problems, this could get really annoying if failing builds because of it and cannot merge because the linter has got it wrong

I'm assuming you pointed those out in the review. If you didn't, please do so or I won't know what to fix.

I pointed out several in the peer review, though I've probably missed loads because of the sheer quantity of files changed. Most of these were 'sentance casing' classnames in headings. Some others so changing the casing of variables because they match a name in a list e.g. $url becoming $URL. My concern here is that the linting is being overly aggressive and getting it wrong. My concern here is that the linting will fail builds because it has gotten casing wrong. That's going to be very annoying and the negative will exceed the benefit gained from continuously checking the casing of docs. I'm not sure how to resolve this. May even need to simply switch this off for CI and just run that particularly bit of linting from a laptop every so often.

[GuySartorelli]

There's a fair number capitilisation problems, this could get really annoying if failing builds because of it and cannot merge because the linter has got it wrong

I pointed out several in the peer review, though I've probably missed loads because of the sheer quantity of files changed. Most of these were 'sentance casing' classnames in headings. Some others so changing the casing of variables because they match a name in a list e.g. $url becoming $URL. My concern here is that the linting is being overly aggressive and getting it wrong.

I really wish there was a way to reply to that type of comment directly on GitHub lol. Anyway:

I think it's fine for us to have a few cases as a result of this PR where casing isn't 100% correct. We can fix them as we find them, but I don't think there will be many left since you and I have both put in some effort to find them already.

In headings, it will be fairly obvious when looking at the docs that a classname was meant to have been surrounded in backticks, and the actual documentation below the heading will be correct regardless.

In the $url vs $URL example, it's very clear what was intended, I don't think it will confuse anyone, and again easy to resolve when we see them.

My concern here is that the linting will fail builds because it has gotten casing wrong. That's going to be very annoying and the negative will exceed the benefit gained from continuously checking the casing of docs. I'm not sure how to resolve this. May even need to simply switch this off for CI and just run that particularly bit of linting from a laptop every so often.

Existing examples have already been linted and are passing linting now. So it would only fail builds for any new documentation. From the examples you found, and the examples I had found while fixing the issues initially, it's basically only wrong when someone forgets backticks. So for new documentation, if it says "the casing is wrong", it's usually going to be a reminder to us that we should be adding backticks.

I really don't think this is as big a problem as you're thinking.

[emteknetnz]

So it would only fail builds for any new documentation

Yeah that's my point, for instance I write a heading ## Adding an extension to LinkFieldAdminController won't it failing linting and tell me it supposed to be ## Adding an extension to linkFieldAdminController?

[GuySartorelli]

So it would only fail builds for any new documentation
Yeah that's my point, for instance I write a heading ## Adding an extension to LinkFieldAdminController won't it failing linting and tell me it supposed to be ## Adding an extension to linkFieldAdminController?

Yes. And as I said:

So for new documentation, if it says "the casing is wrong", it's usually going to be a reminder to us that we should be adding backticks.

i.e. you would see the linting failure, and update your PR to have ## Adding an extension to `LinkFieldAdminController` instead.

[emteknetnz]

OK cool. So what will happen if we put a $url variable to an example code block for new documentation?

[maxime-rainville]

The last two point seem very minor to me.

I generally fall in the "avoid disabling linting rules" camp... especially when those rules are coming from some central source.

If there's still disagreement about this, maybe we have a quick 15 min catch up to go through it.

[maxime-rainville]

I think I've opined on all the outstanding points that were controversial.