Coding Standards Meeting 2023-08-15 [#3375432]

Hello all, it’s time for the fortnightly coding standards meeting.

This meeting:
➤ Is for anyone interested in the Drupal coding standards.
➤ Is held on the #coding standards channel in Drupal Slack (see www.drupal.org/slack for information).
➤ Usually happens fortnightly on Tuesday 2100 UTC in Drupal Slack.
➤ The meeting open for 24 hours to allow for all timezones.
➤ Discussion is done in threads, which you can follow to be notified of new replies even if you don’t comment in the thread. You may also join the meeting later and participate asynchronously.
➤ Has a public agenda anyone by adding a comment to the meeting issue.
➤ A transcript will be made using drupal-meeting-parser and posted to the agenda issue. For anonymous comments, start with a :bust_in_silhouette: emoji. To take a comment or thread off the record, start with a :no_entry_sign: emoji.

Current ping list: @catch, @larowan, @longwave, @quietone
@dww, @borisson_ @longwave @Björn Brala, @Aaron McHale, @Alex Skrypnyk, @Urvashi, @Kingdutch

0️⃣ Who is here today? Comment in the thread to introduce yourself. We’ll keep the meeting open for 24 hours to allow for all timezones.

Alex Skrypnyk	Hi all
catch	:wave:
Björn Brala (bbrala)	Goed morning! :smile:
Kingdutch	Good morning :smile:
dww	Derek (insomnia edition)
urvashi_vora	Hello Everyone :wave:
hestenet (he/him)	Hi all! Tim from the DA :wave::skin-tone-3:
longwave	:wave:

0️⃣.5️⃣ What is your Drupal development environment?

quietone	ddev, with modified ports (via ansible) so that I can have multiple instances up at once. And most instances are setup with multiple database to help with migration work.
catch	ddev. No port trickery though.
Björn Brala (bbrala)	ddev, but struggling with using kubectl as transport for drush :sweat_smile:
Kingdutch	Docker Desktop
Alex Skrypnyk	docker desktop (using drevops.com)
dww	Emacs. MAMP by default. For some projects, Docker Desktop, if needed. Bare iron is so much faster than containers on my dying Intel MacBook Pro.
urvashi_vora	Docker desktop
longwave	ddev

1️⃣ Do you have suggested topics you are looking to discuss? Post in this thread and we’ll open threads for them as appropriate.

2️⃣ Action items

Kingdutch

I added #3309010: Support PHPDoc Types in @param @var @return annotations to the agenda but I think it may have missed the cut :smile:

2️⃣.1️⃣ Approve previous minutes

quietone	#3380936: Coding Standards Meeting 2023-08-01 2100 UTC
quietone	Ask here if you want to do this but want guidance.

2️⃣.2️⃣ Allow constructor methods to omit docblocks #2140961: Allow constructor methods to omit docblocks (edited)

quietone	From #71, has the "required coding standards rule updates issues been attached to the issue"
catch	Core's current coder ruleset allows for no constructor docs, but I think it might be because we don't have a specific rule enabled. Not at computer to check ATM.
longwave	yeah we don't have the rule enabled, #3212066: [meta] Fix Drupal.Commenting.FunctionComment.Missing would bite us here but that is likely a long way off
catch	Yeah I think just open an issue against coder explaining the change and seeing if we can make that sniff selective or something. But we're so far off adopting the rule anyway core can go ahead. (edited)

2️⃣.3️⃣ TBA

3️⃣ Fixed since last meeting

3️⃣.1️⃣ #2140961: Allow constructor methods to omit docblocks

3️⃣.2️⃣ #2707507: Always add trailing commas, for arrays and annotations

3️⃣.4️⃣ #2620756: Hello "coding standards committee"

dww	The CS project page refers to the TWG charter to list the membership of the CS committee. But I thought there was a wider committee (which I volunteered to be on) which aren’t formal TWG members. Don’t we still want/need a formal list of the “active” committee somewhere?
dww	Don’t want to reopen the issue about it, starting here

4️⃣ Update on target issue(s)

quietone	It is RTBC
quietone	Anyone want to suggest a core Coding Standards issue to complete?

5️⃣ Issues tagged "final discussion". These were announced and next step is to approve the change or not. See steps 7 and 8.

5️⃣.1️⃣ #3238192: Allow omitting @var for strictly typed class properties

Björn Brala (bbrala)	Commented in issue. Let's get on board with how ppl work :)
dww	Commented at issue
urvashi_vora	Upvoted on the issue

5️⃣.2️⃣#3324368: Update CSS coding standards to include PostCSS and Drupal 10

Björn Brala (bbrala)	Commented, seems good. Just might ne missing the example of using bem state class in the BEM example. Could be follow up or later change
mherchel	I totally thought this meeting was today :smile: :sweat_smile:
mherchel	I can add that BEM state class
Björn Brala (bbrala)	Only really suggested a followup to not put a dent in the process hehe, but yeah, would be good to add imo

6️⃣ Issues tagged "Needs announcement for final discussion" These are at step 5 and 6. To decide if it can be announced. (edited)

6️⃣.1️⃣ #2464123: Remove the requirement that no blank line follow an inline comment

quietone	I recall having to modify comments for this rule. It was inconvenient. However, I think the benefits outweigh the inconvenience.
quietone	By turning off the we risk having comments disconnected from the code block they refer to.
quietone	This is likely to make it harder for reviewers and core committers. Especially in the case when the writer of the patch likes the blank line but the reviewer thinks it should be removed.
quietone	I much prefer the simpler, if constricting, rule that has a sniff. (edited)
catch	I think bigpipe already has a comment like this, long online comment with a blank line in the middle. So it might not be enforced in core?
borisson_	I think we should keep this rule Removing this rule would lead to more inconsistensies
Alex Skrypnyk	commented on the issue: "allow blank spaces between subsequent comments"
Alex Skrypnyk	@quietonethe issue is marked as RTBC, but I think it requires more discussionI've commented that simply removing the rule will result in a lot of "hanging" comments.pfrenssen has suggested to update the sniff to "detect a blank line that separates a comment from code."Should the issue status be chnaged?

6️⃣.2️⃣ #3074131: Use null coalescing operator ?? instead of a ternary operator with an isset() condition

catch	For me this is a language feature and doesn't need a coder rule. Do we need to stop in people using the old way?
Björn Brala (bbrala)	Well, consistent code is always great, and no better teacher that the ci run telling the user imo. So I'd be al for enforcement.
dww	Consistent and readable code is always great. ?? makes code much more concise, in a way that seems verywidely understood by PHP devs these days. +1 to enforce.
longwave	untested but SlevomatCodingStandard.ControlStructures.RequireNullCoalesceOperator looks relevant here - maybe we could enable it in a trial issue and see how much there is to fix
longwave	i am +1 for changing existing instances in core for readability but weak -1 on enforcing it always
dww	That sniff is already proposed in the issue, right? Seems to Just Work(tm). Also, I believe core already did this conversion. The discussion now is about adopting it as a formal standard and enabling the sniff so it stays that way.
longwave	worth running again to see if any new instances have snuck in since..
Björn Brala (bbrala)	Just remembered this patch. That was fun. #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator

7️⃣ Start time of this meeting.

quietone	I suggest alternating start times, 12 hours apart.
quietone	That would accommodate those in and around NZ/Aus and England.
quietone	If the start times are 9am and 9pm my time I can continue to do the admin for setting up the meeting.
quietone	We just have to ensure that a) I actually start on time and b) the day is not a day that I take my grandson to school. Or, if it is that day, it starts 30 minutes late.
Björn Brala (bbrala)	I'd be fine either way tbh. Lot will be async anyways a lot of times
Kingdutch	:+1:
Alex Skrypnyk	:+1:

8️⃣ Changes made to the Coding Standard wiki pages that have not been approved

quietone	I have come across a few that were done as part of working on a coding standards issue but we not approved by the coding standards committee.
quietone	I have seen such changes reverted but I have not taken that action yet.
quietone	Should they all be reverted, with an explanation in the revision log?
quietone	Should the warning text at the top of those pages change?
Alex Skrypnyk	Without actual examples it is hard to understand which pages were modified and if they should be "protected" by some sort of permissions or just having an explanation text at the top. IMO, as with any rules, only authorised editors should be able to modify the content. And yes, IMO, those changes need to be reverted and taken through a proper process (unless they are spelling fixes).
catch	Can you link to revision diffs or similar for these? Might need an issue to discuss.
urvashi_vora	Agree with @catch.

9️⃣ Critical: Update the coding standards process. #3365085: Update the coding standards process on the project page (edited)

quietone	The current process worked very well for the years the original committee was active.
quietone	Like any process it is worth reviewing to make sure it is well, working for us.
quietone	What do see in the process that can be improved?
Björn Brala (bbrala)	This one I'm gonna remind me on, cause that needs focus to think on.