diff --git a/docs/description.adoc b/docs/description.adoc index f1e82d4854..579a017871 100644 --- a/docs/description.adoc +++ b/docs/description.adoc @@ -71,14 +71,14 @@ This new structure is defined as follows: // This needs to be kept in sync with the [maps in the validation script](https://github.com/SonarSource/rspec/blob/master/rspec-tools/rspec_tools/validation/description.py#L33-L40). * Why is this an issue? ** What is the potential impact? -* How to fix it +* How to fix it (optional) ** Code examples *** Noncompliant code example *** Compliant solution ** How does this work? ** Pitfalls ** Going the extra mile -* How to fix it in {Framework Display Name} +* How to fix it in {Framework Display Name} (optional) ** Code examples *** Noncompliant code example *** Compliant solution @@ -113,7 +113,7 @@ This section can include multiple concrete threats as well. https://github.com/SonarSource/rspec/blob/a51217c6d91abfa5e1d77d0ae0843e3903adf2d0/rules/S3649/impact.adoc[_Example._] + Goal: Our users should understand the impact of this issue on their project. + -* *How to fix it* or *How to fix it in `{Framework Display Name}`* (level 2 title) [the title depends on whether the description is generic or framework-specifc. See examples below.] +* *How to fix it* or *How to fix it in `{Framework Display Name}`* (level 2 title) [Optional; the title depends on whether the description is generic or framework-specifc. See examples below.] + This section consists of one or multiple fixes for this type of issue (`Noncompliant code example` vs. `Compliant solution`). There can be multiple fixes for different libraries and/or frameworks. + Goal: Get an idea of how this issue can be fixed for my project/framework, why this works, what to look out for, and also how to continue improving on this topic. @@ -145,8 +145,7 @@ Goal: Allow the user to dig deeper by providing a curated list of resources. * *Standards* (level 3 title) [Optional] * *Benchmarks* (level 3 title) [Optional] -Note that most sections and subsections are optional, only the `Why is this an issue?` and `How to fix it`/ `How to fix it in {Framework Display Name}` main -sections are mandatory. +Note that most sections and subsections are optional, only the `Why is this an issue?` main section is mandatory. Content of the section "_How to fix it_ / _How to fix it in {Framework Display Name}_" can either be generic or framework specific. diff --git a/rspec-tools/rspec_tools/validation/description.py b/rspec-tools/rspec_tools/validation/description.py index 987e746afa..baf2f471de 100644 --- a/rspec-tools/rspec_tools/validation/description.py +++ b/rspec-tools/rspec_tools/validation/description.py @@ -1,7 +1,6 @@ from bs4 import BeautifulSoup from pathlib import Path from typing import Final -from collections import Counter from rspec_tools.errors import RuleValidationError from rspec_tools.rules import LanguageSpecificRule @@ -31,11 +30,11 @@ ACCEPTED_FRAMEWORK_NAMES: Final[list[str]] = parse_names('docs/header_names/allo # This needs to be kept in sync with the [headers list in docs/descriptions.adoc](https://github.com/SonarSource/rspec/blob/master/docs/description.adoc#3-progressive-education) SECTIONS = { - 'Why is this an issue?': ['What is the potential impact?'], - # Also covers 'How to fix it in {Framework Display Name}' - 'How to fix it': ['Code examples', 'How does this work?', 'Pitfalls', 'Going the extra mile'], + 'Why is this an issue?': ['What is the potential impact?'] } OPTIONAL_SECTIONS = { + # Also covers 'How to fix it in {Framework Display Name}' + 'How to fix it': ['Code examples', 'How does this work?', 'Pitfalls', 'Going the extra mile'], 'Resources': ['Documentation', 'Articles & blog posts', 'Conference presentations', 'Standards', 'Benchmarks'] } SUBSECTIONS = { @@ -55,14 +54,12 @@ def validate_section_names(rule_language: LanguageSpecificRule): education_titles = intersection(h2_titles, list(SECTIONS.keys()) + list(OPTIONAL_SECTIONS.keys())) if education_titles: - # we're using the education format + # Using the education format. validate_how_to_fix_it_sections_names(rule_language, h2_titles) missing_titles = difference(list(SECTIONS.keys()), education_titles) - # we handled "how to fix it" sections above - missing_titles_without_how_to_fix_its = [ s for s in missing_titles if not HOW_TO_FIX_IT_REGEX.match(s)] - if missing_titles_without_how_to_fix_its: - # when using the progressive education format, we need to have all its mandatory titles - raise RuleValidationError(f'Rule {rule_language.id} is missing the "{missing_titles_without_how_to_fix_its[0]}" section') + if missing_titles: + # All mandatory titles have to be present in the rule description. + raise RuleValidationError(f'Rule {rule_language.id} is missing the "{missing_titles[0]}" section') else: # we're using the legacy format for title in h2_titles: @@ -71,10 +68,12 @@ def validate_section_names(rule_language: LanguageSpecificRule): def validate_how_to_fix_it_sections_names(rule_language: LanguageSpecificRule, h2_titles: list[str]): how_to_fix_it_sections = [ s for s in h2_titles if HOW_TO_FIX_IT_REGEX.match(s) ] + if not how_to_fix_it_sections: + # No 'How to fix it' section for the rule. + return if len(how_to_fix_it_sections) > 6: raise RuleValidationError(f'Rule {rule_language.id} has more than 6 "{HOW_TO_FIX_IT}" sections. Please ensure this limit can be increased with PM/UX teams') - if not how_to_fix_it_sections: - raise RuleValidationError(f'Rule {rule_language.id} is missing a "{HOW_TO_FIX_IT}" section') + if HOW_TO_FIX_IT in how_to_fix_it_sections and len(how_to_fix_it_sections) > 1: raise RuleValidationError(f'Rule {rule_language.id} is mixing "{HOW_TO_FIX_IT}" with "How to fix it in FRAMEWORK NAME" sections. Either use a single "{HOW_TO_FIX_IT}" or one or more "How to fix it in FRAMEWORK"') duplicate_names = [x for x in how_to_fix_it_sections if how_to_fix_it_sections.count(x) > 1] # O(n*n) is fine, given n <= 6 @@ -165,12 +164,12 @@ Are you looking for "{highlight_name(rule_language)}"?''') def validate_subsections(rule_language: LanguageSpecificRule): for optional_section in list(OPTIONAL_SECTIONS.keys()): - validate_subsections_for_section(rule_language, optional_section, OPTIONAL_SECTIONS[optional_section]) - for mandatory_section in list(SECTIONS.keys()): - if mandatory_section == HOW_TO_FIX_IT: - validate_subsections_for_section(rule_language, mandatory_section, SECTIONS[mandatory_section], section_regex=HOW_TO_FIX_IT_REGEX) + if optional_section == HOW_TO_FIX_IT: + validate_subsections_for_section(rule_language, optional_section, OPTIONAL_SECTIONS[optional_section], section_regex=HOW_TO_FIX_IT_REGEX) else: - validate_subsections_for_section(rule_language, mandatory_section, SECTIONS[mandatory_section]) + validate_subsections_for_section(rule_language, optional_section, OPTIONAL_SECTIONS[optional_section]) + for mandatory_section in list(SECTIONS.keys()): + validate_subsections_for_section(rule_language, mandatory_section, SECTIONS[mandatory_section]) for subsection_with_sub_subsection in list(SUBSECTIONS.keys()): if subsection_with_sub_subsection == 'Code examples': validate_subsections_for_section(rule_language, subsection_with_sub_subsection, SUBSECTIONS[subsection_with_sub_subsection], level=4, is_duplicate_allowed=True) diff --git a/rspec-tools/tests/resources/invalid-rules/S200/php/rule.adoc b/rspec-tools/tests/resources/rules/S200/php/rule.adoc similarity index 100% rename from rspec-tools/tests/resources/invalid-rules/S200/php/rule.adoc rename to rspec-tools/tests/resources/rules/S200/php/rule.adoc diff --git a/rspec-tools/tests/resources/invalid-rules/S200/php/rule.html b/rspec-tools/tests/resources/rules/S200/php/rule.html similarity index 100% rename from rspec-tools/tests/resources/invalid-rules/S200/php/rule.html rename to rspec-tools/tests/resources/rules/S200/php/rule.html diff --git a/rspec-tools/tests/validation/test_description_validation.py b/rspec-tools/tests/validation/test_description_validation.py index 2804a06018..d5d21b8ac5 100644 --- a/rspec-tools/tests/validation/test_description_validation.py +++ b/rspec-tools/tests/validation/test_description_validation.py @@ -147,12 +147,6 @@ def test_education_format_missing_mandatory_sections_validation(invalid_rule): with pytest.raises(RuleValidationError, match=f'Rule common:S200 is missing the "Why is this an issue\\?" section'): validate_section_names(rule) -def test_education_missing_how_to_fix_it_validation(invalid_rule): - '''Check that missing the "How to fix it" in the education format breaks validation''' - rule = invalid_rule('S200', 'php') - with pytest.raises(RuleValidationError, match=f'Rule php:S200 is missing a "How to fix it" section'): - validate_section_names(rule) - def test_code_examples_with_typo_validation(invalid_rule): '''Check that the "Code examples" subsection with a typo in the education format breaks validation''' rule = invalid_rule('S200', 'cobol') @@ -165,6 +159,11 @@ def test_noncompliant_examples_with_typo_validation(invalid_rule): with pytest.raises(RuleValidationError, match=f'Rule apex:S200 has a "Code examples" subsection with an unallowed name: "Non-compliant example"'): validate_subsections(rule) +def test_valid_optional_how_to_fix_it_section_validation(rule_language): + '''Check that missing the "How to fix it" section in the education format is considered valid''' + rule = rule_language('S200', 'php') + validate_section_names(rule) + def test_valid_how_to_fix_it_subsections_validation(rule_language): '''Check that expected format is considered valid''' rule = rule_language('S101', 'csharp')