From 5111509577337fac3f46b5e804798e23e1dfd0d0 Mon Sep 17 00:00:00 2001 From: Vadim Peretokin Date: Sun, 27 Nov 2022 22:41:08 +0100 Subject: [PATCH] Infrastructure: trial new guideline - no TODOs as they dont actually add value (#6446) #### Brief overview of PR changes/additions Trial a new project guideline for the codebase - no TODOs in the code. #### Motivation for adding to Mudlet We've added 100+ TODOs to the codebase over the 10+ years of Mudlet and have barely resolved any of them. They just don't add any value by sitting around, and nobody historically has taken an interest in going through and resolving them. #### Other info (issues closed, discussion etc) Let's revisit on 1 Jan 2023 if this guideline still makes sense and we'd like to keep it. --- .github/CONTRIBUTING.md | 10 +++++----- dangerfile.js | 13 +------------ 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 68e551146..441c88ae1 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -70,6 +70,11 @@ Don't: "

Another paragraph, maybe in a different style, e.g. italics or bold."))); ``` +# TODO's +In Mudlet's decade of existence, TODO's get added to the codebase but almost never resolved. Having them in the codebase does not add any value if they are just sitting there and not acted upon. This raises the question, why have them around at all if they are not adding any value? To this effect, the project decided not to add future TODOs to the code and work on removing existing ones. + +In case the TODO is an improvement, a pull request implementing the idea or an issue suggesting it can be added - no TODO necessary. In case it's a legitimate problem, it's better to fix it right away instead of kicking the can down the road. + # Git commit guidelines for core team ## Refactoring @@ -83,11 +88,6 @@ Don't: * PR Title must start with `fix`, `improve`, `add`, or `infra` * This facilitates automatic changelog gathering and categorization * Cannot merge until it is fixed: core team can always adjust it before merging -* Any new TODO in a source file must have a Mudlet github issue on the same line - * good: - * //TODO: https://github.com/Mudlet/Mudlet/issues/1234 - * bad: - * //TODO: a thing we aren't actually tracking with an issue Danger will also give a heads up if the PR title is long, or if more than 10 source files are changed in a single PR. These are not blocked but the warnings should serve to draw attention to something which may require a double check. More info below. diff --git a/dangerfile.js b/dangerfile.js index d8d65b792..ecf52463a 100644 --- a/dangerfile.js +++ b/dangerfile.js @@ -1,6 +1,4 @@ const {danger, fail, message, warn} = require('danger'); -const ISSUE_REGEX = /https?:\/\/(?:www\.)?github\.com\/Mudlet\/Mudlet\/issues\/(\d+)/i -const ISSUE_URL = "https://github.com/Mudlet/Mudlet/issues" const SOURCE_REGEX = /.*\.(cpp|c|h|lua)$/i const TITLE_REGEX = /^(fix|improve|add|infra)/i const touched_files = [...danger.git.created_files, ...danger.git.modified_files] @@ -21,20 +19,11 @@ if (title_type) { sourcefiles.forEach(function(filename) { const additions = danger.git.diffForFile(filename) additions.then(diff => { - const issues = [] diff.added.split("\n").forEach(function(item) { if (item.includes("TODO:")) { - const has_issue = item.match(ISSUE_REGEX) - if (!has_issue) { - fail(`Source file ${filename} includes a TODO with no Mudlet issue link.\n New TODO items in source files must have an accompanying github issue`) - } else { - issues.push(has_issue[1]) - } + fail(`Source file ${filename} includes a TODO! Can you make the change right away? If no, better not to create a TODO - they just tend to hang around and never get fixed.`) } }) - if (issues.length > 0) { - message(`\`${filename}\` adds TODO issues: ${issues.map(iss => `[${iss}](${ISSUE_URL}/${iss})`).join(", ")}`,{icon: ":heavy_check_mark:"} ) - } }) })