From bcf97b608b3a46d71d4c081a3bb838163cc67f8a Mon Sep 17 00:00:00 2001 From: Dunedan Date: Thu, 29 Aug 2024 14:35:53 +0200 Subject: [PATCH] Enable ruff rules for docstrings and comments This enables some ruff rules for docstrings and comments. The idea is to not enforce the presence of docstrings, but to ensure they are properly formatted if they're present. For comments this adds checks that they don't contain code and verify the formatting of comments with "TODO" tags. As part of this, some commented out code which hasn't been touch in the past 10 years gets removed as well. The rules enabled enabled by this are: - check formatting of existing docstrings (D200-) - check comments for code (ERA) - check formatting of TODO tags (TD001, TD004-) --- ruff.toml | 6 ++--- source/tools/entity/scriptlib/__init__.py | 15 +++++------ source/tools/fontbuilder2/Packer.py | 21 +++++++-------- source/tools/fontbuilder2/fontbuilder.py | 10 +------ source/tools/i18n/checkDiff.py | 8 +++--- source/tools/i18n/cleanTranslationFiles.py | 3 ++- source/tools/i18n/creditTranslators.py | 7 ++--- source/tools/i18n/extractors/extractors.py | 6 ++--- source/tools/i18n/generateDebugTranslation.py | 8 +++--- source/tools/i18n/i18n_helper/catalog.py | 2 +- source/tools/i18n/i18n_helper/globber.py | 4 +-- source/tools/i18n/updateTemplates.py | 4 +-- source/tools/templatesanalyzer/unitTables.py | 27 ++++++++++--------- source/tools/xmlvalidator/validate_grammar.py | 11 ++++---- 14 files changed, 63 insertions(+), 69 deletions(-) diff --git a/ruff.toml b/ruff.toml index 489c964f84..481df22744 100644 --- a/ruff.toml +++ b/ruff.toml @@ -9,10 +9,9 @@ ignore = [ "ANN", "C90", "COM812", - "D", + "D10", "DTZ005", "EM", - "ERA", "FA", "FIX", "FBT", @@ -36,7 +35,8 @@ ignore = [ "S603", "S607", "T20", - "TD", + "TD002", + "TD003", "TRY002", "TRY003", "TRY004", diff --git a/source/tools/entity/scriptlib/__init__.py b/source/tools/entity/scriptlib/__init__.py index e69ba9e1a5..5c6af3b7da 100644 --- a/source/tools/entity/scriptlib/__init__.py +++ b/source/tools/entity/scriptlib/__init__.py @@ -33,9 +33,7 @@ class SimulTemplateEntity: return main_mod def apply_layer(self, base_tag, tag): - """ - apply tag layer to base_tag - """ + """Apply tag layer to base_tag.""" if tag.get("datatype") == "tokens": base_tokens = split(r"\s+", base_tag.text or "") tokens = split(r"\s+", tag.text or "") @@ -89,9 +87,7 @@ class SimulTemplateEntity: return entity def _load_inherited(self, base_path, vfs_path, mods, base=None): - """ - vfs_path should be relative to base_path in a mod - """ + # vfs_path should be relative to base_path in a mod if "|" in vfs_path: paths = vfs_path.split("|", 1) base = self._load_inherited(base_path, paths[1], mods, base) @@ -119,15 +115,16 @@ class SimulTemplateEntity: def find_files(vfs_root, mods, vfs_path, *ext_list): - """ - returns a list of 2-size tuple with: + """Find files. + + Returns a list of 2-size tuple with: - Path relative to the mod base - full Path """ full_exts = ["." + ext for ext in ext_list] def find_recursive(dp, base): - """(relative Path, full Path) generator""" + """(relative Path, full Path) generator.""" if dp.is_dir(): if dp.name not in (".svn", ".git") and not dp.name.endswith("~"): for fp in dp.iterdir(): diff --git a/source/tools/fontbuilder2/Packer.py b/source/tools/fontbuilder2/Packer.py index 40a1758439..0ae2907486 100644 --- a/source/tools/fontbuilder2/Packer.py +++ b/source/tools/fontbuilder2/Packer.py @@ -29,12 +29,12 @@ class Point: self.y = y def __cmp__(self, other): - """Compares the starting position of height slices""" + """Compare the starting position of height slices.""" return self.x - other.x class RectanglePacker: - """Base class for rectangle packing algorithms + """Base class for rectangle packing algorithms. By uniting all rectangle packers under this common base class, you can easily switch between different algorithms to find the most efficient or @@ -45,7 +45,7 @@ class RectanglePacker: """ def __init__(self, packingAreaWidth, packingAreaHeight): - """Initializes a new rectangle packer + """Initialize a new rectangle packer. packingAreaWidth: Maximum width of the packing area packingAreaHeight: Maximum height of the packing area @@ -54,7 +54,7 @@ class RectanglePacker: self.packingAreaHeight = packingAreaHeight def Pack(self, rectangleWidth, rectangleHeight): - """Allocates space for a rectangle in the packing area + """Allocate space for a rectangle in the packing area. rectangleWidth: Width of the rectangle to allocate rectangleHeight: Height of the rectangle to allocate @@ -69,7 +69,7 @@ class RectanglePacker: return point def TryPack(self, rectangleWidth, rectangleHeight): - """Tries to allocate space for a rectangle in the packing area + """Try to allocate space for a rectangle in the packing area. rectangleWidth: Width of the rectangle to allocate rectangleHeight: Height of the rectangle to allocate @@ -102,8 +102,7 @@ class DumbRectanglePacker(RectanglePacker): class CygonRectanglePacker(RectanglePacker): - """ - Packer using a custom algorithm by Markus 'Cygon' Ewald + """Packer using a custom algorithm by Markus 'Cygon' Ewald. Algorithm conceived by Markus Ewald (cygon at nuclex dot org), though I'm quite sure I'm not the first one to come up with it :) @@ -120,7 +119,7 @@ class CygonRectanglePacker(RectanglePacker): """ def __init__(self, packingAreaWidth, packingAreaHeight): - """Initializes a new rectangle packer + """Initialize a new rectangle packer. packingAreaWidth: Maximum width of the packing area packingAreaHeight: Maximum height of the packing area @@ -134,7 +133,7 @@ class CygonRectanglePacker(RectanglePacker): self.heightSlices.append(Point(0, 0)) def TryPack(self, rectangleWidth, rectangleHeight): - """Tries to allocate space for a rectangle in the packing area + """Try to allocate space for a rectangle in the packing area. rectangleWidth: Width of the rectangle to allocate rectangleHeight: Height of the rectangle to allocate @@ -160,7 +159,7 @@ class CygonRectanglePacker(RectanglePacker): return placement def tryFindBestPlacement(self, rectangleWidth, rectangleHeight): - """Finds the best position for a rectangle of the given dimensions + """Find the best position for a rectangle of the given dimensions. rectangleWidth: Width of the rectangle to find a position for rectangleHeight: Height of the rectangle to find a position for @@ -234,7 +233,7 @@ class CygonRectanglePacker(RectanglePacker): return Point(self.heightSlices[bestSliceIndex].x, bestSliceY) def integrateRectangle(self, left, width, bottom): - """Integrates a new rectangle into the height slice table + """Integrate a new rectangle into the height slice table. left: Position of the rectangle's left side width: Width of the rectangle diff --git a/source/tools/fontbuilder2/fontbuilder.py b/source/tools/fontbuilder2/fontbuilder.py index 5d2a2ebb47..54faeb4172 100755 --- a/source/tools/fontbuilder2/fontbuilder.py +++ b/source/tools/fontbuilder2/fontbuilder.py @@ -50,11 +50,6 @@ class Glyph: self.w = bb[2] - bb[0] self.h = bb[3] - bb[1] - # Force multiple of 4, to avoid leakage across S3TC blocks - # (TODO: is this useful?) - # self.w += (4 - (self.w % 4)) % 4 - # self.h += (4 - (self.h % 4)) % 4 - def pack(self, packer): self.pos = packer.Pack(self.w, self.h) @@ -114,7 +109,7 @@ def generate_font(outname, ttfNames, loadopts, size, renderstyle, dsizes): (ctx, _) = setup_context(1, 1, renderstyle) - # TODO this gets the line height from the default font + # TODO: this gets the line height from the default font # while entire texts can be in the fallback font ctx.set_font_face(faceList[0]) ctx.set_font_size(size + dsizes[ttfNames[0]]) @@ -155,7 +150,6 @@ def generate_font(outname, ttfNames, loadopts, size, renderstyle, dsizes): # Using the dump pacher usually creates bigger textures, but runs faster # In practice the size difference is so small it always ends up in the same size packer = Packer.DumbRectanglePacker(w, h) - # packer = Packer.CygonRectanglePacker(w, h) for g in glyphs: g.pack(packer) except Packer.OutOfSpaceError: @@ -174,8 +168,6 @@ def generate_font(outname, ttfNames, loadopts, size, renderstyle, dsizes): fnt.write("%d\n" % len(glyphs)) fnt.write("%d\n" % linespacing) fnt.write("%d\n" % charheight) - # sorting unneeded, as glyphs are added in increasing order - # glyphs.sort(key = lambda g: ord(g.char)) for g in glyphs: x0 = g.x0 y0 = g.y0 diff --git a/source/tools/i18n/checkDiff.py b/source/tools/i18n/checkDiff.py index 8dc79fcf34..342518600a 100755 --- a/source/tools/i18n/checkDiff.py +++ b/source/tools/i18n/checkDiff.py @@ -27,7 +27,7 @@ from i18n_helper import projectRootDirectory def get_diff(): - """Return a diff using svn diff""" + """Return a diff using svn diff.""" os.chdir(projectRootDirectory) diff_process = subprocess.run(["svn", "diff", "binaries"], capture_output=True, check=False) @@ -38,7 +38,9 @@ def get_diff(): def check_diff(diff: io.StringIO) -> List[str]: - """Run through a diff of .po files and check that some of the changes + """Check a diff of .po files for meaningful changes. + + Run through a diff of .po files and check that some of the changes are real translations changes and not just noise (line changes....). The algorithm isn't extremely clever, but it is quite fast. """ @@ -97,7 +99,7 @@ def revert_files(files: List[str], verbose=False): def add_untracked(verbose=False): - """Add untracked .po files to svn""" + """Add untracked .po files to svn.""" diff_process = subprocess.run(["svn", "st", "binaries"], capture_output=True, check=False) if diff_process.stderr != b"": print(f"Error running svn st: {diff_process.stderr.decode('utf-8')}. Exiting.") diff --git a/source/tools/i18n/cleanTranslationFiles.py b/source/tools/i18n/cleanTranslationFiles.py index dede3f11ab..780694d75f 100755 --- a/source/tools/i18n/cleanTranslationFiles.py +++ b/source/tools/i18n/cleanTranslationFiles.py @@ -16,7 +16,8 @@ # You should have received a copy of the GNU General Public License # along with 0 A.D. If not, see . -""" +"""Remove unnecessary personal data of translators. + This file removes unneeded personal data from the translators. Most notably the e-mail addresses. We need to translators' nicks for the credits, but no more data is required. diff --git a/source/tools/i18n/creditTranslators.py b/source/tools/i18n/creditTranslators.py index ee6ef12cb6..3938774463 100755 --- a/source/tools/i18n/creditTranslators.py +++ b/source/tools/i18n/creditTranslators.py @@ -16,9 +16,10 @@ # You should have received a copy of the GNU General Public License # along with 0 A.D. If not, see . -""" -This file updates the translators credits located in the public mod GUI files, using -translators names from the .po files. +"""Update the translator credits. + +This file updates the translator credits located in the public mod GUI files, using +translator names from the .po files. If translators change their names on Transifex, the script will remove the old names. TODO: It should be possible to add people in the list manually, and protect them against diff --git a/source/tools/i18n/extractors/extractors.py b/source/tools/i18n/extractors/extractors.py index c2154c746a..c1a723e271 100644 --- a/source/tools/i18n/extractors/extractors.py +++ b/source/tools/i18n/extractors/extractors.py @@ -32,7 +32,7 @@ from textwrap import dedent def pathmatch(mask, path): - """Matches paths to a mask, where the mask supports * and **. + """Match paths to a mask, where the mask supports * and **. Paths use / as the separator * matches a sequence of characters without /. @@ -66,7 +66,7 @@ class Extractor: self.excludeMasks = [] def run(self): - """Extracts messages. + """Extract messages. :return: An iterator over ``(message, plural, context, (location, pos), comment)`` tuples. @@ -106,7 +106,7 @@ class Extractor: yield message, plural, context, (filename, position), comments def extractFromFile(self, filepath): - """Extracts messages from a specific file. + """Extract messages from a specific file. :return: An iterator over ``(message, plural, context, position, comments)`` tuples. :rtype: ``iterator`` diff --git a/source/tools/i18n/generateDebugTranslation.py b/source/tools/i18n/generateDebugTranslation.py index 32937e1a86..9b246f8ef9 100755 --- a/source/tools/i18n/generateDebugTranslation.py +++ b/source/tools/i18n/generateDebugTranslation.py @@ -30,8 +30,8 @@ DEBUG_PREFIX = "X_X " def generate_long_strings(root_path, input_file_name, output_file_name, languages=None): - """ - Generate the 'long strings' debug catalog. + """Generate the 'long strings' debug catalog. + This catalog contains the longest singular and plural string, found amongst all translated languages or a filtered subset. It can be used to check if GUI elements are large enough. @@ -104,8 +104,8 @@ def generate_long_strings(root_path, input_file_name, output_file_name, language def generate_debug(root_path, input_file_name, output_file_name): - """ - Generate a debug catalog to identify untranslated strings. + """Generate a debug catalog to identify untranslated strings. + This prefixes all strings with DEBUG_PREFIX, to easily identify untranslated strings while still making the game navigable. The catalog is debug.*.po diff --git a/source/tools/i18n/i18n_helper/catalog.py b/source/tools/i18n/i18n_helper/catalog.py index dbe818ba5f..8c8c092d33 100644 --- a/source/tools/i18n/i18n_helper/catalog.py +++ b/source/tools/i18n/i18n_helper/catalog.py @@ -1,4 +1,4 @@ -"""Wrapper around babel Catalog / .po handling""" +"""Wrapper around babel Catalog / .po handling.""" from datetime import datetime diff --git a/source/tools/i18n/i18n_helper/globber.py b/source/tools/i18n/i18n_helper/globber.py index 20dd7402bf..f473b48459 100644 --- a/source/tools/i18n/i18n_helper/globber.py +++ b/source/tools/i18n/i18n_helper/globber.py @@ -1,4 +1,4 @@ -"""Utils to list .po""" +"""Utils to list .po.""" import os from typing import List, Optional @@ -7,7 +7,7 @@ from i18n_helper.catalog import Catalog def getCatalogs(inputFilePath, filters: Optional[List[str]] = None) -> List[Catalog]: - """Returns a list of "real" catalogs (.po) in the given folder.""" + """Return a list of "real" catalogs (.po) in the given folder.""" existingTranslationCatalogs = [] l10nFolderPath = os.path.dirname(inputFilePath) inputFileName = os.path.basename(inputFilePath) diff --git a/source/tools/i18n/updateTemplates.py b/source/tools/i18n/updateTemplates.py index 680d14168a..413cc5eca0 100755 --- a/source/tools/i18n/updateTemplates.py +++ b/source/tools/i18n/updateTemplates.py @@ -29,9 +29,7 @@ messagesFilename = "messages.json" def warnAboutUntouchedMods(): - """ - Warn about mods that are not properly configured to get their messages extracted. - """ + """Warn about mods that are not properly configured to get their messages extracted.""" modsRootFolder = os.path.join(projectRootDirectory, "binaries", "data", "mods") untouchedMods = {} for modFolder in os.listdir(modsRootFolder): diff --git a/source/tools/templatesanalyzer/unitTables.py b/source/tools/templatesanalyzer/unitTables.py index dbda80ed65..65dbf7f39c 100755 --- a/source/tools/templatesanalyzer/unitTables.py +++ b/source/tools/templatesanalyzer/unitTables.py @@ -374,7 +374,8 @@ def SortFn(A): def WriteColouredDiff(file, diff, isChanged): - """Helper to write coloured text. + """Help to write coloured text. + diff value must always be computed as a unit_spec - unit_generic. A positive imaginary part represents advantageous trait. """ @@ -504,16 +505,18 @@ def computeTemplates(LoadTemplatesIfParent): def computeCivTemplates(Civs: list): - """Load Civ specific templates""" - # NOTE: whether a Civ can train a certain unit is not recorded in the unit - # .xml files, and hence we have to get that info elsewhere, e.g. from the - # Civ tree. This should be delayed until this whole parser is based on the - # Civ tree itself. + """Load Civ specific templates. - # This function must always ensure that Civ unit parenthood works as - # intended, i.e. a unit in a Civ indeed has a 'Civ' field recording its - # loyalty to that Civ. Check this when upgrading this script to keep - # up with the game engine. + NOTE: whether a Civ can train a certain unit is not recorded in the unit + .xml files, and hence we have to get that info elsewhere, e.g. from the + Civ tree. This should be delayed until this whole parser is based on the + Civ tree itself. + + This function must always ensure that Civ unit parenthood works as + intended, i.e. a unit in a Civ indeed has a 'Civ' field recording its + loyalty to that Civ. Check this when upgrading this script to keep + up with the game engine. + """ pwd = os.getcwd() os.chdir(basePath) @@ -557,7 +560,7 @@ def computeCivTemplates(Civs: list): def computeTemplatesByParent(templates: dict, Civs: list, CivTemplates: dict): - """Get them in the array""" + """Get them in the array.""" # Civs:list -> CivTemplates:dict -> templates:dict -> TemplatesByParent TemplatesByParent = {} for Civ in Civs: @@ -590,7 +593,7 @@ efficiencyTable = computeUnitEfficiencyDiff(TemplatesByParent, Civs) ############################################################ def writeHTML(): - """Create the HTML file""" + """Create the HTML file.""" f = open( os.path.realpath(__file__).replace("unitTables.py", "") + "unit_summary_table.html", "w", diff --git a/source/tools/xmlvalidator/validate_grammar.py b/source/tools/xmlvalidator/validate_grammar.py index c6dbf81c3c..3d956ec8d8 100755 --- a/source/tools/xmlvalidator/validate_grammar.py +++ b/source/tools/xmlvalidator/validate_grammar.py @@ -65,22 +65,23 @@ class RelaxNGValidator: return not self.inError def main(self): - """Program entry point, parses command line arguments and launches the validation""" + """Program entry point, parses command line arguments and launches the validation.""" # ordered uniq mods (dict maintains ordered keys from python 3.6) self.logger.info("Checking %s's integrity.", "|".join(self.mods)) self.logger.info("The following mods will be loaded: %s.", "|".join(self.mods)) return self.run() def find_files(self, vfs_root, mods, vfs_path, *ext_list): - """ - returns a list of 2-size tuple with: + """Find files. + + Returns a list of 2-size tuple with: - Path relative to the mod base - full Path """ full_exts = ["." + ext for ext in ext_list] def find_recursive(dp, base): - """(relative Path, full Path) generator""" + """(relative Path, full Path) generator.""" if dp.is_dir(): if dp.name not in (".svn", ".git") and not dp.name.endswith("~"): for fp in dp.iterdir(): @@ -179,7 +180,7 @@ class RelaxNGValidator: return realpath(join(self.vfs_root, mod_name, vfs_path)) def get_relaxng_file(self, schemapath): - """We look for the highest priority mod relax NG file""" + """Get the highest priority mod relax NG file.""" for mod in self.mods: relax_ng_path = self.get_physical_path(mod, schemapath) if exists(relax_ng_path):