mmo-ref/mudlet
1
0
Fork 0
mirror of https://github.com/Mudlet/Mudlet synced 2026-08-13 18:26:27 -04:00
Code Issues Projects Releases Packages Wiki Activity

Infrastructure: trial new guideline - no TODOs as they dont actually add value (#6446)

Browse source 
<!-- Keep the title short & concise so anyone non-technical can
understand it,
     the title appears in PTB changelogs -->
#### 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.
This commit is contained in:
Vadim Peretokin 2022-11-27 22:41:08 +01:00 • committed by GitHub
parent cdb35e2c9b
commit 5111509577
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 6 additions and 17 deletions
Download patch file Download diff file Expand all files Collapse all files

10
.github/CONTRIBUTING.md vendored
View file

@ -70,6 +70,11 @@ Don't:
"<p>Another paragraph, maybe in a different style, e.g. <i>italics</i> or <b>bold</b>.")));
```
# 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.

13
dangerfile.js
View file

@ -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:"} )
}
})
})