From ea647067f0bb96e7feab8272d367c3d4e7d6eda4 Mon Sep 17 00:00:00 2001 From: Dunedan Date: Tue, 27 Aug 2024 19:28:11 +0200 Subject: [PATCH] Enable ruff rules to check for ambiguous code This enables some ruff rules to check for ambiguous and dead Python code, which might cause unintended side-effects. The enabled rules are: - a bunch of rules related to shadowing of builtin structures (A) - a bunch of rules checking for unused arguments (ARG) - a rule checking for useless expressions (B018) - a rule checking for unbound loop variables (B023) - a rule checking redefined function parameters (PLR1704) --- ruff.toml | 5 ---- source/collada/tests/tests.py | 16 ++++++------ source/tools/entity/checkrefs.py | 11 ++++---- source/tools/entity/entvalidate.py | 2 +- source/tools/fontbuilder2/fontbuilder.py | 4 +-- source/tools/i18n/checkDiff.py | 4 +-- source/tools/i18n/updateTemplates.py | 2 +- .../rlclient/python/samples/simple-example.py | 14 +++++----- .../rlclient/python/tests/test_actions.py | 26 +++++++++---------- .../rlclient/python/zero_ad/environment.py | 12 +++++---- source/tools/spirv/compile.py | 5 +--- source/tools/templatesanalyzer/unitTables.py | 12 ++++----- source/tools/xmlvalidator/validate_grammar.py | 3 +-- source/tools/xmlvalidator/validator.py | 3 +-- 14 files changed, 55 insertions(+), 64 deletions(-) diff --git a/ruff.toml b/ruff.toml index b1d6a61148..7192cfae63 100644 --- a/ruff.toml +++ b/ruff.toml @@ -6,11 +6,7 @@ line-ending = "lf" [lint] select = ["ALL"] ignore = [ - "A", - "ARG", "ANN", - "B018", - "B023", "C90", "COM812", "D", @@ -27,7 +23,6 @@ ignore = [ "PLR0912", "PLR0913", "PLR0915", - "PLR1704", "PLR2004", "PLW2901", "PT", diff --git a/source/collada/tests/tests.py b/source/collada/tests/tests.py index 3bfeec9c79..c033d3c952 100755 --- a/source/collada/tests/tests.py +++ b/source/collada/tests/tests.py @@ -37,8 +37,8 @@ library.set_skeleton_definitions(skeleton_definitions, len(skeleton_definitions) def _convert_dae(func, filename, expected_status=0): output = [] - def cb(cbdata, str, len): - output.append(string_at(str, len)) + def cb(_, ptr, size): + output.append(string_at(ptr, size)) cbtype = CFUNCTYPE(None, POINTER(None), POINTER(c_char), c_uint) status = func(filename, cbtype(cb), None) @@ -127,9 +127,9 @@ for test_file in ["xsitest3c", "xsitest3e", "jav2d", "jav2d2"]: input_filename = f"{test_data}/{test_file}.dae" output_filename = f"{test_mod}/art/meshes/{test_file}.pmd" - input = open(input_filename).read() - output = convert_dae_to_pmd(input) - open(output_filename, "wb").write(output) + file_input = open(input_filename).read() + file_output = convert_dae_to_pmd(file_input) + open(output_filename, "wb").write(file_output) xml = create_actor( test_file, @@ -155,6 +155,6 @@ for test_file in ["xsitest3c", "xsitest3e", "jav2d", "jav2d2"]: input_filename = f"{test_data}/{test_file}.dae" output_filename = f"{test_mod}/art/animation/{test_file}.psa" - input = open(input_filename).read() - output = convert_dae_to_psa(input) - open(output_filename, "wb").write(output) + file_input = open(input_filename).read() + file_output = convert_dae_to_psa(file_input) + open(output_filename, "wb").write(file_output) diff --git a/source/tools/entity/checkrefs.py b/source/tools/entity/checkrefs.py index afbf9cd4f0..b3da2281a4 100755 --- a/source/tools/entity/checkrefs.py +++ b/source/tools/entity/checkrefs.py @@ -36,10 +36,9 @@ class CheckRefs: self.supportedAnimationFormats = ("psa", "dae") self.supportedAudioFormats = "ogg" self.mods = [] - self.__init_logger + self.__init_logger() self.inError = False - @property def __init_logger(self): logger = getLogger(__name__) logger.setLevel(INFO) @@ -398,7 +397,7 @@ class CheckRefs: reqTag = cmp_identity.find("Requirements") if reqTag is not None: - def parse_requirements(req, recursionDepth=1): + def parse_requirements(fp, req, recursionDepth=1): techsTag = req.find("Techs") if techsTag is not None: for techTag in techsTag.text.split(): @@ -410,12 +409,12 @@ class CheckRefs: recursionDepth -= 1 allReqTag = req.find("All") if allReqTag is not None: - parse_requirements(allReqTag, recursionDepth) + parse_requirements(fp, allReqTag, recursionDepth) anyReqTag = req.find("Any") if anyReqTag is not None: - parse_requirements(anyReqTag, recursionDepth) + parse_requirements(fp, anyReqTag, recursionDepth) - parse_requirements(reqTag) + parse_requirements(fp, reqTag) cmp_researcher = entity.find("Researcher") if cmp_researcher is not None: diff --git a/source/tools/entity/entvalidate.py b/source/tools/entity/entvalidate.py index ba07f11358..51bbd547e2 100755 --- a/source/tools/entity/entvalidate.py +++ b/source/tools/entity/entvalidate.py @@ -20,7 +20,7 @@ Please create the file: {} You can do that by running 'pyrogenesis -dumpSchema' in the 'system' directory """ XMLLINT_ERROR_MSG = ( - "xmllint not found in your PATH, please install it " "(usually in libxml2 package)" + "xmllint not found in your PATH, please install it (usually in libxml2 package)" ) diff --git a/source/tools/fontbuilder2/fontbuilder.py b/source/tools/fontbuilder2/fontbuilder.py index 1d5d207520..4f01cd1f26 100755 --- a/source/tools/fontbuilder2/fontbuilder.py +++ b/source/tools/fontbuilder2/fontbuilder.py @@ -95,8 +95,8 @@ def load_char_list(filename): # Construct a Cairo context and surface for rendering text with the given parameters def setup_context(width, height, renderstyle): - format = cairo.FORMAT_ARGB32 if "colour" in renderstyle else cairo.FORMAT_A8 - surface = cairo.ImageSurface(format, width, height) + surface_format = cairo.FORMAT_ARGB32 if "colour" in renderstyle else cairo.FORMAT_A8 + surface = cairo.ImageSurface(surface_format, width, height) ctx = cairo.Context(surface) ctx.set_line_join(cairo.LINE_JOIN_ROUND) return ctx, surface diff --git a/source/tools/i18n/checkDiff.py b/source/tools/i18n/checkDiff.py index 2f84061baa..8dc79fcf34 100755 --- a/source/tools/i18n/checkDiff.py +++ b/source/tools/i18n/checkDiff.py @@ -37,7 +37,7 @@ def get_diff(): return io.StringIO(diff_process.stdout.decode("utf-8")) -def check_diff(diff: io.StringIO, verbose=False) -> List[str]: +def check_diff(diff: io.StringIO) -> List[str]: """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. @@ -126,6 +126,6 @@ if __name__ == "__main__": parser = argparse.ArgumentParser() parser.add_argument("--verbose", help="Print reverted files.", action="store_true") args = parser.parse_args() - need_revert = check_diff(get_diff(), args.verbose) + need_revert = check_diff(get_diff()) revert_files(need_revert, args.verbose) add_untracked(args.verbose) diff --git a/source/tools/i18n/updateTemplates.py b/source/tools/i18n/updateTemplates.py index 88d5e90e12..84f4b61236 100755 --- a/source/tools/i18n/updateTemplates.py +++ b/source/tools/i18n/updateTemplates.py @@ -48,7 +48,7 @@ def warnAboutUntouchedMods(): f"in the root folder of this mod." ) if untouchedMods: - print("" "Warning: No messages were extracted from the following mods:" "") + print("Warning: No messages were extracted from the following mods:") for mod in untouchedMods: print(f"• {mod}: {untouchedMods[mod]}") print( diff --git a/source/tools/rlclient/python/samples/simple-example.py b/source/tools/rlclient/python/samples/simple-example.py index 5b00a821e6..920ed805ab 100644 --- a/source/tools/rlclient/python/samples/simple-example.py +++ b/source/tools/rlclient/python/samples/simple-example.py @@ -42,14 +42,14 @@ state = game.reset(arcadia_config) state = game.step() # Units can be queried from the game state -citizen_soldiers = state.units(owner=1, type="infantry") +citizen_soldiers = state.units(owner=1, entity_type="infantry") # (including gaia units like trees or other resources) -nearby_tree = closest(state.units(owner=0, type="tree"), center(citizen_soldiers)) +nearby_tree = closest(state.units(owner=0, entity_type="tree"), center(citizen_soldiers)) # Action commands can be created using zero_ad.actions collect_wood = zero_ad.actions.gather(citizen_soldiers, nearby_tree) -female_citizens = state.units(owner=1, type="female_citizen") +female_citizens = state.units(owner=1, entity_type="female_citizen") house_tpl = "structures/spart/house" x = 680 z = 640 @@ -69,7 +69,7 @@ print("female citizen's max health is", female_citizen.max_health()) print(female_citizen.data) # Units can be built using the "train action" -civic_center = state.units(owner=1, type="civil_centre")[0] +civic_center = state.units(owner=1, entity_type="civil_centre")[0] spearman_type = "units/spart/infantry_spearman_b" train_spearmen = zero_ad.actions.train([civic_center], spearman_type) @@ -96,11 +96,11 @@ for _ in range(150): # Let's attack with our entire military state = game.step([zero_ad.actions.chat("An attack is coming!")]) -while len(state.units(owner=2, type="unit")) > 0: +while len(state.units(owner=2, entity_type="unit")) > 0: attack_units = [ - unit for unit in state.units(owner=1, type="unit") if "female" not in unit.type() + unit for unit in state.units(owner=1, entity_type="unit") if "female" not in unit.type() ] - target = closest(state.units(owner=2, type="unit"), center(attack_units)) + target = closest(state.units(owner=2, entity_type="unit"), center(attack_units)) state = game.step([zero_ad.actions.attack(attack_units, target)]) while state.unit(target.id()): diff --git a/source/tools/rlclient/python/tests/test_actions.py b/source/tools/rlclient/python/tests/test_actions.py index deab900d48..7e0617d232 100644 --- a/source/tools/rlclient/python/tests/test_actions.py +++ b/source/tools/rlclient/python/tests/test_actions.py @@ -33,23 +33,23 @@ def closest(units, position): def test_construct(): state = game.reset(config) - female_citizens = state.units(owner=1, type="female_citizen") + female_citizens = state.units(owner=1, entity_type="female_citizen") house_tpl = "structures/spart/house" - house_count = len(state.units(owner=1, type=house_tpl)) + house_count = len(state.units(owner=1, entity_type=house_tpl)) x = 680 z = 640 build_house = zero_ad.actions.construct(female_citizens, house_tpl, x, z, autocontinue=True) # Check that they start building the house state = game.step([build_house]) - while len(state.units(owner=1, type=house_tpl)) == house_count: + while len(state.units(owner=1, entity_type=house_tpl)) == house_count: state = game.step() def test_gather(): state = game.reset(config) - female_citizen = state.units(owner=1, type="female_citizen")[0] - state.units(owner=0, type="tree") - nearby_tree = closest(state.units(owner=0, type="tree"), female_citizen.position()) + female_citizen = state.units(owner=1, entity_type="female_citizen")[0] + state.units(owner=0, entity_type="tree") + nearby_tree = closest(state.units(owner=0, entity_type="tree"), female_citizen.position()) collect_wood = zero_ad.actions.gather([female_citizen], nearby_tree) state = game.step([collect_wood]) @@ -59,19 +59,19 @@ def test_gather(): def test_train(): state = game.reset(config) - civic_centers = state.units(owner=1, type="civil_centre") + civic_centers = state.units(owner=1, entity_type="civil_centre") spearman_type = "units/spart/infantry_spearman_b" - spearman_count = len(state.units(owner=1, type=spearman_type)) + spearman_count = len(state.units(owner=1, entity_type=spearman_type)) train_spearmen = zero_ad.actions.train(civic_centers, spearman_type) state = game.step([train_spearmen]) - while len(state.units(owner=1, type=spearman_type)) == spearman_count: + while len(state.units(owner=1, entity_type=spearman_type)) == spearman_count: state = game.step() def test_walk(): state = game.reset(config) - female_citizens = state.units(owner=1, type="female_citizen") + female_citizens = state.units(owner=1, entity_type="female_citizen") x = 680 z = 640 initial_distance = dist(center(female_citizens), [x, z]) @@ -81,14 +81,14 @@ def test_walk(): distance = initial_distance while distance >= initial_distance: state = game.step() - female_citizens = state.units(owner=1, type="female_citizen") + female_citizens = state.units(owner=1, entity_type="female_citizen") distance = dist(center(female_citizens), [x, z]) def test_attack(): state = game.reset(config) - unit = state.units(owner=1, type="cavalry")[0] - target = state.units(owner=2, type="female_citizen")[0] + unit = state.units(owner=1, entity_type="cavalry")[0] + target = state.units(owner=2, entity_type="female_citizen")[0] initial_health_target = target.health() initial_health_unit = unit.health() diff --git a/source/tools/rlclient/python/zero_ad/environment.py b/source/tools/rlclient/python/zero_ad/environment.py index 4acb00e2c8..e349654e7f 100644 --- a/source/tools/rlclient/python/zero_ad/environment.py +++ b/source/tools/rlclient/python/zero_ad/environment.py @@ -58,18 +58,20 @@ class GameState: self.game = game self.mapSize = self.data["mapSize"] - def units(self, owner=None, type=None): + def units(self, owner=None, entity_type=None): def filter_fn(e): return (owner is None or e["owner"] == owner) and ( - type is None or type in e["template"] + entity_type is None or entity_type in e["template"] ) return [Entity(e, self.game) for e in self.data["entities"].values() if filter_fn(e)] - def unit(self, id): - id = str(id) + def unit(self, entity_id): + entity_id = str(entity_id) return ( - Entity(self.data["entities"][id], self.game) if id in self.data["entities"] else None + Entity(self.data["entities"][entity_id], self.game) + if entity_id in self.data["entities"] + else None ) diff --git a/source/tools/spirv/compile.py b/source/tools/spirv/compile.py index b14fad0cbc..3b27ffcf1d 100755 --- a/source/tools/spirv/compile.py +++ b/source/tools/spirv/compile.py @@ -88,9 +88,7 @@ def resolve_if(defines, expression): return False -def compile_and_reflect( - input_mod_path, output_mod_path, dependencies, stage, path, out_path, defines -): +def compile_and_reflect(input_mod_path, dependencies, stage, path, out_path, defines): keep_debug = False input_path = os.path.normpath(path) output_path = os.path.normpath(out_path) @@ -443,7 +441,6 @@ def build(rules, input_mod_path, output_mod_path, dependencies, program_name): reflection = compile_and_reflect( input_mod_path, - output_mod_path, dependencies, shader["type"], input_glsl_path, diff --git a/source/tools/templatesanalyzer/unitTables.py b/source/tools/templatesanalyzer/unitTables.py index e541d0fd38..5f81f3ae5c 100755 --- a/source/tools/templatesanalyzer/unitTables.py +++ b/source/tools/templatesanalyzer/unitTables.py @@ -192,8 +192,8 @@ def CalcUnit(UnitName, existingUnit=None): resource_cost = Template.find("./Cost/Resources") if resource_cost is not None: - for type in list(resource_cost): - unit["Cost"][type.tag] = ExtractValue(type) + for resource_type in list(resource_cost): + unit["Cost"][resource_type.tag] = ExtractValue(resource_type) if Template.find("./Attack/Melee") is not None: unit["RepeatRate"]["Melee"] = ExtractValue(Template.find("./Attack/Melee/RepeatTime")) @@ -499,7 +499,7 @@ def computeTemplates(LoadTemplatesIfParent): return templates -def computeCivTemplates(template: dict, Civs: list): +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 @@ -524,8 +524,8 @@ def computeCivTemplates(template: dict, Civs: list): if os.path.isfile(template): # filter based on FilterOut breakIt = False - for filter in FilterOut: - if template.find(filter) != -1: + for civ_filter in FilterOut: + if template.find(civ_filter) != -1: breakIt = True if breakIt: continue @@ -577,7 +577,7 @@ def computeTemplatesByParent(templates: dict, Civs: list, CivTemplates: dict): ############################################################ ## Pre-compute all tables templates = computeTemplates(LoadTemplatesIfParent) -CivTemplates = computeCivTemplates(templates, Civs) +CivTemplates = computeCivTemplates(Civs) TemplatesByParent = computeTemplatesByParent(templates, Civs, CivTemplates) # Not used; use it for your own custom analysis diff --git a/source/tools/xmlvalidator/validate_grammar.py b/source/tools/xmlvalidator/validate_grammar.py index 6a18136047..c6dbf81c3c 100755 --- a/source/tools/xmlvalidator/validate_grammar.py +++ b/source/tools/xmlvalidator/validate_grammar.py @@ -31,10 +31,9 @@ class RelaxNGValidator: def __init__(self, vfs_root, mods=None, verbose=False): self.mods = mods if mods is not None else [] self.vfs_root = Path(vfs_root) - self.__init_logger + self.__init_logger() self.verbose = verbose - @property def __init_logger(self): logger = getLogger(__name__) logger.setLevel(INFO) diff --git a/source/tools/xmlvalidator/validator.py b/source/tools/xmlvalidator/validator.py index 8775e96d32..bff311326d 100755 --- a/source/tools/xmlvalidator/validator.py +++ b/source/tools/xmlvalidator/validator.py @@ -94,9 +94,8 @@ class Validator: self.materials = {} self.invalid_materials = {} self.actors = [] - self.__init_logger + self.__init_logger() - @property def __init_logger(self): logger = getLogger(__name__) logger.setLevel(INFO)