Closed (fixed)
Project:
Coding Standards
Component:
Meeting
Priority:
Normal
Category:
Plan
Assigned:
Unassigned
Reporter:
Created:
10 Oct 2023 at 21:53 UTC
Updated:
22 Nov 2023 at 07:14 UTC
Jump to comment: Most recent
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. Alternating between Tuesday 2100 UTC and Wednesday 0900 UTC.
➤ The meeting open for 24 hours to allow for all time zones.
➤ 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 time zones.
| quietone | Hi! |
| urvashi_vora | Hello |
| Björn Brala (bbrala) | Hi |
| gapple | :wave: |
| catch | here late. |
1️⃣ Do you have suggested topics you are looking to discuss? Post in this thread and we’ll open threads for them as appropriate.
| quietone | Blog post, #3380948: Draft blog announcement 2023-09-07 |
| urvashi_vora | @quietone, I somewhere saw a blog post, having little introduction about the newly added members, can we do the same here? or the document is fine with just the names? |
2️⃣ Action items
| quietone | @quietone to post announcement of new members and process |
| quietone | @quietone to likely post #3380948: Draft blog announcement 2023-09-07 |
2️⃣.1️⃣ Approve previous minutes
| larowlan | Already approved #3391801: Coding Standards Meeting Tuesday 2023-10-10 2100 UTC |
3️⃣ We now have a calendar for the meetings and are going to take it in turns for admin duties. Who wants to take the next meeting?
| larowlan | The calendar is here, thanks for setting it up quietone https://calendar.google.com/calendar/u/0?cid=ODNkZjViOWIwY2Q0M2U3YjUwYWF... |
| larowlan | My turn this fortnight |
| quietone | I think the recurring for the meetings should be every two weeks and not the first and last weekday. |
| urvashi_vora | I agree, I see them scheduled once in a month |
| quietone | Oh, that is interesting. I have two in each month. |
| larowlan | I set them to be monthly on nth day |
| urvashi_vora | I have next meeting scheduled for 22-Nov |
| larowlan | But I can make them use 4 weekly |
| larowlan | I have updated them |
| urvashi_vora | Great, looks good now. |
| quietone | Thanks @larowlan |
| urvashi_vora | Also, I would like to host the next meeting. |
| larowlan | Might be worth us working out the TZs |
| larowlan | They are at 7am and 7pm for me so the alternative one would suit me better than this one (7pm). Might be the converse for you urvashi |
| urvashi_vora | For me its showing 2:30-3:30AM in GMT, which indicates 08:00-09:00AM IST, but today's meeting is happening at 2:30PM IST for me. Thus, I am confused about the exact meeting time |
| larowlan | I wonder if I got the times wrong then |
| urvashi_vora | Probably:sweat_smile: |
| urvashi_vora | Do we have a time decided for the meetings? |
| larowlan | I used the times from the channel description |
| larowlan | https://drupal.slack.com/archives/C02LJCF78E8/p1696978807122519 |
| urvashi_vora | 2100 UTC means 9 PM UTC (on Tuesdays), and 0900 UTC is 9 AM UTC (on Wednesdays), right? |
| larowlan | So this is the 9am Wednesday one |
| urvashi_vora | Yes |
| urvashi_vora | At present, we have all the meetings on Wednesdays, as per this timezone. |
| larowlan | 2100 UTC Tuesday is Wed for anything east of GMT+3, so for both of us that's Wed |
| urvashi_vora | Yeah |
4️⃣ our process is now marked fixed #3365085: Update the coding standards process on the project page :tada:
5️⃣ publishing our draft blog post
| larowlan | #3380948: Draft blog announcement 2023-09-07 |
| quietone | So, I have once again let this slip. |
| larowlan | Who can post this? Gabor? |
| quietone | I suggest that after the 24 hours, I post this. And that means at the next meeting there will be two issues to discuss. |
| quietone | I have permission to post. |
| larowlan | Yes if you can post it after the 24 hours that would be great |
| quietone | Great. If anyone objects comment here. I will look before I post. |
| quietone | Oh, I may not be available to post in 24 hours. It could be more like 36. |
| larowlan | I could post if I have access |
| larowlan | Not seeing blog at node/add so guess I don't |
| larowlan | Perhaps we could save a draft and have it published |
| Gábor Hojtsy (he/him) | Please post it, you don’t need me for it :smile: |
| quietone | So, this draft blog post has 2 issues. In this meeting there are 3 issues being discussed, and if the committee approves of them, they need to be included in the blog post. That means, at the next meeting there will be 5 issues at Step 6. |
| quietone | And there could also be RTBCed issues to discuss (Step 4). |
| quietone | It could be a fair bit of work. |
| Gábor Hojtsy (he/him) | Oh btw there was also a blog post in the works for https://www.drupal.org/about/core/blog to announce the new members and process. Is it possible to get that one out too? 🙂 I think it would highlight the process changes and people and potentially encourage more to contribute 🙂 |
| Gábor Hojtsy (he/him) | that is following up on #3378689: Add more coding standards maintainers, part II |
| Gábor Hojtsy (he/him) | Post for review / publication is at https://docs.google.com/document/d/1C5iJUQ2OlmIYVXcCOTYJnGXrnB0gyDPoXiwi..., I don’t know if the changes explained are complete or not 🙂 |
| quietone | The post for new members and process is up, https://www.drupal.org/about/core |
| quietone | And the regular blog post about issue review is published too, https://www.drupal.org/about/core/blog/coding-standards-proposals-for-fi... |
| larowlan | Thanks! |
| larowlan | I'll do the minutes tomorrow |
6️⃣ rtbc issues
6️⃣ .1️⃣ #1624564: Coding standards for "use" statements
| quietone | Just skimming the issue. I see that the documentation has been updated even though the change was not formally approved. |
| larowlan | So we should revert the docs change? |
| quietone | We probably should so this can go through the process correctly. |
| quietone | As for the issue itself, it says the sorting in not case sensitive. That seems wrong to me. |
| larowlan | Yeah the sort order should be deterministic to minimise merge conflicts right |
| catch | That was chosen due to phpstorm, but phpstorm should follow coding standards not dictate them. I did this for now: #1624564: Coding standards for "use" statements#comment-15290241 |
| quietone | I am working on reverting the doc changes now. |
| quietone | Revert complete. |
6️⃣.2️⃣ #3295249: Allow multi-line function declarations
| quietone | This is a good idea for constructors, they can be terribly long. It doesn't seem to bother me for other methods but it would be odd to do this for some methods and not others. |
| larowlan | Needs a CR too |
| quietone | We should also think about how this would be implemented so we avoid issues that make the change by file. This seems like one that can be automated. |
| larowlan | I've already added a +1 on the issue |
| larowlan | Will add a CR tomorrow |
| quietone | I was just reviewing an issue that was converting some constructors to multi-line as part of adding a new parameter. I found it added work to the review process. |
| larowlan | Yeah normal scope rules apply |
| quietone | I updated the Issue Summary here to use the new template. In doing so I now see that the suggested wording is from the PSR and that uses the capitalized words suchs as MUST and MAY. That is not what our standards use. |
| quietone | There is an issue to use that style, which is an RFC. #1795750: Revise coding standards to use IETF RFC 2119 standards |
| quietone | The recommendation is also defining the format for the return type, which is a separate issue, #2928856: Adopt the PSR-12 standard for PHP return types |
| quietone | Using the new template does help to ensure an issue is ready! |
| quietone | I am wondering if this should go back to Needs Review. I will take a break though. |
| larowlan | feels like it needs some extra work under the new process |
| catch | If we just allow it, we shouldn't need a load of core issues to change it - should mainly be for new code or extreme cases we want to change. |
6️⃣.3️⃣ #2355209: Clearly mark functions that should not be used outside of their project
| larowlan | This seems reasonable |
| larowlan | It's missing a change record, I can add that tomorrow |
| urvashi_vora | That would be great @larowlan |
| quietone | Agg, the IS is not up to date, but fortunately there are only 9 comments |
| quietone | The recommending wording needs to be changed to not use 'please'. |
| quietone | I updated to the new template. There are 2 clear supporters instead of 3, I am not sure jhodgdon counts. |
| quietone | Plus the proposal did not have an example, which I added. |
| quietone | So, I think this is also back to NR. |
| quietone | Or maybe needs work? |
| larowlan | sounds like NW |
| catch | I'm not convinced this is worth the work, we shouldn't be adding new private functions to core or contrib code these days anyway. Would rather discourage their use entirely. |
| quietone | I set this to needs review to discuss the catch's point above. |
| Kingdutch | I'm not convinced this is worth the work, we shouldn't be adding new private functions to core or contrib code these days anyway. Would rather discourage their use entirely.I respectfully disagree. The alternative I’ve seen teams use rather than single-focus functions is a helper class, which just screams “OOP for the sake of it” and generally leads tothings that are worse tech debt than a function would’ve been. There’s definitely a place for regular functions that encapsulate some business logic. If written properly they’re even very easy to unit test (that’s basically the most basic PHPUnit example). Having a standardized way of marking those as internal is a good thing. |
| Kingdutch | (Currently out of office catching up on some Slack, can copy paste this to the issue later this week) |
| catch | I agree with this for say a helper you call from hook_node_update/hook_node_insert() and I prefer that to 25 lines of boilerplate to do the same in a class, but #3366083: [META] Hooks via attributes on service methods (hux style) will deprecate procedural hooks, and then what's left? |
| Kingdutch | Sidetrack: Didn’t realize that was going to be moved into core already. Also interesting my biggest gripe with hooks is that they’re not namespaced; not that they’re not on classes :face_with_hand_over_mouth: I think I proposed in a random place somewhere to namespace module/install files and remove the module specific function naming, which would solve all my own problems.On-track: I have for sure written non-hook functions to perform some task in a Drupal codebase, that would probably still be around. But I’d have to go search for them on a computer if you want examples.An example that does come to mind are actually functions like those in the Amplibrary to work with multiple fibers (e.g. to race or wait for all). Basically logic that fits in a pure function on some object(s).Debates can be had for all of them whether the code needs to be changeable and whether it should thus be a service. I suspect in contrib/custom code that answer is “no” more often than in the case of core (or more core-like contrib — since my mind doesn’t see all of contrib as equal with some more clearly serving a library than an end-purpose module — I see it as a spectrum) |
That's all folks for the meeting facilitation. Keep chatting in the threads and feel free to add new ones. (edited)
| quietone | @larowlan thank you for facilitating and setting up the calendar |
| urvashi_vora | Thank you @larowlan |
| Björn Brala (bbrala) | ❤️ |
Comments
Comment #7
larowlanComment #8
urvashi_vora commentedThe minutes looks good and are verified from the slack conversation. Adding a few more replied from the thread from 6️⃣.3️⃣, those were added after the meeting.
Marking it as RTBC and Fixed.
Comment #9
urvashi_vora commented