Skip to content

Commit

Permalink
Adds CI to restrict types in a file (ParadiseSS13#24694)
Browse files Browse the repository at this point in the history
* LETS GAMBLE!

* oops

* even more oops

* alright lets fuckin do it

* real names please

* comment

* Contributing.md update

* maybe someone will find this todo

* actually lets make it a define so people can't mistype it

* review

* changes
  • Loading branch information
Contrabang authored Apr 19, 2024
1 parent a31b7d4 commit a95373f
Show file tree
Hide file tree
Showing 18 changed files with 147 additions and 32 deletions.
21 changes: 20 additions & 1 deletion .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,26 @@ SS13 has a lot of legacy code that's never been updated. Here are some examples

- Files and path accessed and referenced by code above simply being #included should be strictly lowercase to avoid issues on filesystems where case matters.

#### Modular Code in a File

Code should be modular where possible; if you are working on a new addition, then strongly consider putting it in its own file unless it makes sense to put it with similar ones (i.e. a new tool would go in the `tools.dm` file)

Our codebase also has support for checking files so that they only contain one specific typepath, including none of its subtypes. This can be done by adding a specific header at the beginning of the file, which the CI will look for when running. An example can be seen below. You can also run this test locally using `/tools/ci/restrict_file_types.py`

```dm
RESTRICT_TYPE(/datum/foo)
/datum/proc/do_thing() // Error: '/datum' proc found in a file restricted to '/datum/foo'
/datum/foo
/datum/foo/do_thing()
/datum/foo/bar // Error: '/datum/foo/bar' type definition found in a file restricted to '/datum/foo'
/datum/foo/bar/do_thing() // Error: '/datum/foo/bar' proc found in a file restricted to '/datum/foo'
```

### SQL

- Do not use the shorthand sql insert format (where no column names are specified) because it unnecessarily breaks all queries on minor column changes and prevents using these tables for tracking outside related info such as in a connected site/forum.
Expand Down Expand Up @@ -679,7 +699,6 @@ SS13 has a lot of legacy code that's never been updated. Here are some examples

### Other Notes

- Code should be modular where possible; if you are working on a new addition, then strongly consider putting it in its own file unless it makes sense to put it with similar ones (i.e. a new tool would go in the `tools.dm` file)
- Bloated code may be necessary to add a certain feature, which means there has to be a judgement over whether the feature is worth having or not. You can help make this decision easier by making sure your code is modular.

- You are expected to help maintain the code that you add, meaning that if there is a problem then you are likely to be approached in order to fix any issues, runtimes, or bugs.
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ jobs:
python tools/ci/unticked_files.py ${GITHUB_WORKSPACE}
python tools/ci/illegal_dme_files.py ${GITHUB_WORKSPACE}
python tools/ci/define_sanity.py
python tools/ci/restrict_file_types.py
python -m tools.ci.check_icon_conflicts
python -m tools.ci.check_icon_dupenames
python -m tools.maplint.source --github
Expand Down
7 changes: 7 additions & 0 deletions code/__DEFINES/antag_defines.dm
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,10 @@ GLOBAL_LIST(contractors)
* Pulse Demon
*/
#define PULSEDEMON_SOURCE_DRAIN_INVALID (-1)

/proc/ischangeling(mob/M) // TODO: Someone please convert these to proper defines some day.
return M.mind?.has_antag_datum(/datum/antagonist/changeling)

// Helper proc that determines if a mob is a mindslave.
/proc/ismindslave(mob/living/carbon/human/H)
return istype(H) && H.mind.has_antag_datum(/datum/antagonist/mindslave, FALSE)
3 changes: 3 additions & 0 deletions code/__DEFINES/misc_defines.dm
Original file line number Diff line number Diff line change
Expand Up @@ -695,3 +695,6 @@ do { \
#define TEAM_ADMIN_ADD_OBJ_SUCCESS (1<<0)
#define TEAM_ADMIN_ADD_OBJ_CANCEL_LOG (1<<1)
#define TEAM_ADMIN_ADD_OBJ_PURPOSEFUL_CANCEL (1<<2)

/// A helper used by `restrict_file_types.py` to identify types to restrict in a file. Not used by byond at all.
#define RESTRICT_TYPE(type) // do nothing
4 changes: 4 additions & 0 deletions code/_onclick/hud/hud_datum.dm
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,7 @@

/datum/hud/proc/update_locked_slots()
return

/datum/hud/proc/remove_vampire_hud()
static_inventory -= vampire_blood_display
QDEL_NULL(vampire_blood_display)
2 changes: 2 additions & 0 deletions code/modules/antagonists/_common/antag_datum.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
RESTRICT_TYPE(/datum/antagonist)

GLOBAL_LIST_EMPTY(antagonists)

#define SUCCESSFUL_DETACH "dont touch this string numbnuts"
Expand Down
5 changes: 2 additions & 3 deletions code/modules/antagonists/changeling/datum_changeling.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
RESTRICT_TYPE(/datum/antagonist/changeling)

/datum/antagonist/changeling
name = "Changeling"
roundend_category = "changelings"
Expand Down Expand Up @@ -440,8 +442,5 @@
else
to_chat(L, "<span class='notice'>While our current form may be lifeless, this is not the end for us as we can still regenerate!</span>")

/proc/ischangeling(mob/M)
return M.mind?.has_antag_datum(/datum/antagonist/changeling)

/datum/antagonist/changeling/custom_blurb()
return "We awaken on the [station_name()], [get_area_name(owner.current, TRUE)]...\nWe have our tasks to attend to..."
2 changes: 2 additions & 0 deletions code/modules/antagonists/cult/datum_cultist.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
RESTRICT_TYPE(/datum/antagonist/cultist)

/datum/antagonist/cultist
name = "Cultist"
job_rank = ROLE_CULTIST
Expand Down
2 changes: 2 additions & 0 deletions code/modules/antagonists/cult/team_cult.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
RESTRICT_TYPE(/datum/team/cult)

/datum/team/cult
name = "Cult"
antag_datum_type = /datum/antagonist/cultist
Expand Down
1 change: 1 addition & 0 deletions code/modules/antagonists/revolutionary/datum_headrev.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
RESTRICT_TYPE(/datum/antagonist/rev/head)

/datum/antagonist/rev/head
name = "Head Revolutionary"
Expand Down
2 changes: 2 additions & 0 deletions code/modules/antagonists/revolutionary/datum_revolutionary.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
RESTRICT_TYPE(/datum/antagonist/rev)

/datum/antagonist/rev
name = "Revolutionary"
roundend_category = "revs"
Expand Down
2 changes: 2 additions & 0 deletions code/modules/antagonists/revolutionary/team_revolution.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
RESTRICT_TYPE(/datum/team/revolution)

/datum/team/revolution
name = "Revolution"
antag_datum_type = /datum/antagonist/rev
Expand Down
5 changes: 1 addition & 4 deletions code/modules/antagonists/traitor/datum_mindslave.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
RESTRICT_TYPE(/datum/antagonist/mindslave)

// For Mindslaves and Zealots
/datum/antagonist/mindslave
Expand Down Expand Up @@ -87,7 +88,3 @@
slaved.serv -= owner
slaved.leave_serv_hud(owner)
owner.som = null

// Helper proc that determines if a mob is a mindslave.
/proc/ismindslave(mob/living/carbon/human/H)
return istype(H) && H.mind.has_antag_datum(/datum/antagonist/mindslave, FALSE)
1 change: 1 addition & 0 deletions code/modules/antagonists/traitor/datum_traitor.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
RESTRICT_TYPE(/datum/antagonist/traitor)

// Syndicate Traitors.
/datum/antagonist/traitor
Expand Down
27 changes: 3 additions & 24 deletions code/modules/antagonists/vampire/vamp_datum.dm
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
RESTRICT_TYPE(/datum/antagonist/vampire)

/datum/antagonist/vampire
name = "Vampire"
antag_hud_type = ANTAG_HUD_VAMPIRE
Expand Down Expand Up @@ -33,27 +35,6 @@
blurb_b = 138
blurb_a = 1

/datum/antagonist/mindslave/thrall
name = "Vampire Thrall"
antag_hud_type = ANTAG_HUD_VAMPIRE
antag_hud_name = "vampthrall"

/datum/antagonist/mindslave/thrall/add_owner_to_gamemode()
SSticker.mode.vampire_enthralled += owner

/datum/antagonist/mindslave/thrall/remove_owner_from_gamemode()
SSticker.mode.vampire_enthralled -= owner

/datum/antagonist/mindslave/thrall/apply_innate_effects(mob/living/mob_override)
mob_override = ..()
var/datum/mind/M = mob_override.mind
M.AddSpell(new /datum/spell/vampire/thrall_commune)

/datum/antagonist/mindslave/thrall/remove_innate_effects(mob/living/mob_override)
mob_override = ..()
var/datum/mind/M = mob_override.mind
M.RemoveSpell(/datum/spell/vampire/thrall_commune)

/datum/antagonist/vampire/Destroy(force, ...)
owner.current.create_log(CONVERSION_LOG, "De-vampired")
draining = null
Expand Down Expand Up @@ -361,9 +342,7 @@
mob_override.dna?.species.hunger_icon = 'icons/mob/screen_hunger_vampire.dmi'
check_vampire_upgrade(FALSE)

/datum/hud/proc/remove_vampire_hud()
static_inventory -= vampire_blood_display
QDEL_NULL(vampire_blood_display)


/datum/antagonist/vampire/custom_blurb()
return "On the date [GLOB.current_date_string], at [station_time_timestamp()],\n in the [station_name()], [get_area_name(owner.current, TRUE)]...\nThe hunt begins again..."
22 changes: 22 additions & 0 deletions code/modules/antagonists/vampire/vamp_thrall.dm
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
RESTRICT_TYPE(/datum/antagonist/mindslave/thrall)

/datum/antagonist/mindslave/thrall
name = "Vampire Thrall"
antag_hud_type = ANTAG_HUD_VAMPIRE
antag_hud_name = "vampthrall"

/datum/antagonist/mindslave/thrall/add_owner_to_gamemode()
SSticker.mode.vampire_enthralled += owner

/datum/antagonist/mindslave/thrall/remove_owner_from_gamemode()
SSticker.mode.vampire_enthralled -= owner

/datum/antagonist/mindslave/thrall/apply_innate_effects(mob/living/mob_override)
mob_override = ..()
var/datum/mind/M = mob_override.mind
M.AddSpell(new /datum/spell/vampire/thrall_commune)

/datum/antagonist/mindslave/thrall/remove_innate_effects(mob/living/mob_override)
mob_override = ..()
var/datum/mind/M = mob_override.mind
M.RemoveSpell(/datum/spell/vampire/thrall_commune)
1 change: 1 addition & 0 deletions paradise.dme
Original file line number Diff line number Diff line change
Expand Up @@ -1469,6 +1469,7 @@
#include "code\modules\antagonists\traitor\contractor\items\contractor_uplink.dm"
#include "code\modules\antagonists\traitor\contractor\items\extraction_items.dm"
#include "code\modules\antagonists\vampire\vamp_datum.dm"
#include "code\modules\antagonists\vampire\vamp_thrall.dm"
#include "code\modules\antagonists\vampire\vampire_subclasses.dm"
#include "code\modules\antagonists\vampire\vampire_powers\dantalion_powers.dm"
#include "code\modules\antagonists\vampire\vampire_powers\gargantua_powers.dm"
Expand Down
71 changes: 71 additions & 0 deletions tools/ci/restrict_file_types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import glob
import os
import re
import sys
import time
from collections import namedtuple

Failure = namedtuple("Failure", ["filename", "lineno", "message"])

RED = "\033[0;31m"
GREEN = "\033[0;32m"
BLUE = "\033[0;34m"
NC = "\033[0m" # No Color

def print_error(message: str, filename: str, line_number: int):
if os.getenv("GITHUB_ACTIONS") == "true": # We're on github, output in a special format.
print(f"::error file={filename},line={line_number},title=Restricted Type in File::{filename}:{line_number}: {RED}{message}{NC}")
else:
print(f"{filename}:{line_number}: {RED}{message}{NC}")

if __name__ == "__main__":
print("restrict_file_types started")

exit_code = 0
start = time.time()

dm_files = glob.glob("**/*.dm", recursive=True)

if len(sys.argv) > 1:
dm_files = sys.argv[1:]

all_failures = []

for code_filepath in dm_files:
with open(code_filepath, encoding="UTF-8") as code:
filename = code_filepath.split(os.path.sep)[-1]

restrict_regex = re.match(r"RESTRICT_TYPE\((.+)\)", code.read())
if(not restrict_regex):
continue

restrict_type_path = restrict_regex.group(1)

# Matches a definition into two groups:
# 1: The typepath or /proc (for global procs), required. First character is handled specially, to avoid picking up start-of-line comments.
# 2: The name of the proc, if any.
definition_matcher = re.compile(r'^(/[\w][\w/]*?)(?:/proc)?(?:/([\w]+)\(.*?)?(?: */[/*].*)?$')
code.seek(0)
for idx, line in enumerate(code):
if(rematch_result := re.search(definition_matcher, line)):
if(restrict_type_path != rematch_result.group(1)):
type_path = rematch_result.group(1)
proc_name = rematch_result.group(2)
if(type_path == "/proc"):
all_failures += [Failure(code_filepath, idx + 1, f"'Global proc '/proc/{proc_name}' found in a file restricted to type '{restrict_type_path}'")]
else:
if(proc_name):
all_failures += [Failure(code_filepath, idx + 1, f"'Proc '{type_path}/proc/{proc_name}' found in a file restricted to type '{restrict_type_path}'")]
else:
all_failures += [Failure(code_filepath, idx + 1, f"'Definition for different type '{type_path}' found in a file restricted to '{restrict_type_path}'")]


if all_failures:
exit_code = 1
for failure in all_failures:
print_error(failure.message, failure.filename, failure.lineno)

end = time.time()
print(f"restrict_file_types tests completed in {end - start:.2f}s\n")

sys.exit(exit_code)

0 comments on commit a95373f

Please sign in to comment.