Skip to content

Commit

Permalink
fix: Propagate app_name to DjanoSearch, fix naming
Browse files Browse the repository at this point in the history
The app_name option should always filter on the whole DjangoSearch when
present, not just the report. This fixes that. Also updates some
variable and output names to be more clear about the state of the
models.
  • Loading branch information
bmtcril committed Nov 1, 2024
1 parent c456911 commit 20dc66d
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 23 deletions.
11 changes: 9 additions & 2 deletions code_annotations/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def entry_point():
show_default=True,
help='List all locally defined models (in the current repo) that require annotations.',
)
@click.option('--app_name', default='', help='(Optional) App name for which coverage is generated.')
@click.option('--app_name', default=None, help='(Optional) App name for which coverage is generated.')
@click.option('--report_path', default=None, help='Location to write the report')
@click.option('-v', '--verbosity', count=True, help='Verbosity level (-v through -vvv)')
@click.option('--lint/--no_lint', help='Enable or disable linting checks', default=False, show_default=True)
Expand All @@ -61,9 +61,16 @@ def django_find_annotations(
Subcommand for dealing with annotations in Django models.
"""
try:
if not coverage and not seed_safelist and not list_local_models and not lint and not report and not coverage:
click.echo(
"No actions specified. Please specify one or more of --seed_safelist, --list_local_models, "
"--lint, --report, or --coverage"
)
sys.exit(1)

start_time = datetime.datetime.now()
config = AnnotationConfig(config_file, report_path, verbosity)
searcher = DjangoSearch(config)
searcher = DjangoSearch(config, app_name)

# Early out if we're trying to do coverage, but a coverage target is not configured
if coverage and not config.coverage_target:
Expand Down
36 changes: 17 additions & 19 deletions code_annotations/find_django.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,27 @@ class DjangoSearch(BaseSearch):
Handles Django model comment searching for annotations.
"""

def __init__(self, config):
def __init__(self, config, app_name=None):
"""
Initialize for DjangoSearch.
Args:
config: Configuration file path
"""
super().__init__(config)
self.local_models, self.non_local_models, total, needing_annotation = self.get_models_requiring_annotations()
self.local_models, self.non_local_models, total, annotation_eligible = self.get_models_requiring_annotations(app_name)
self.model_counts = {
'total': total,
'annotated': 0,
'unannotated': 0,
'needing_annotation': len(needing_annotation),
'not_needing_annotation': total - len(needing_annotation),
'annotation_eligible': len(annotation_eligible),
'not_annotation_eligible': total - len(annotation_eligible),
'safelisted': 0
}
self.uncovered_model_ids = set()
self.echo.echo_vvv('Local models:\n ' + '\n '.join([str(m) for m in self.local_models]) + '\n')
self.echo.echo_vvv('Non-local models:\n ' + '\n '.join([str(m) for m in self.non_local_models]) + '\n')
self.echo.echo_vv('The following models require annotations:\n ' + '\n '.join(needing_annotation) + '\n')
self.echo.echo_vv('The following models require annotations:\n ' + '\n '.join(annotation_eligible) + '\n')

def _increment_count(self, count_type, incr_by=1):
self.model_counts[count_type] += incr_by
Expand Down Expand Up @@ -82,7 +82,7 @@ def seed_safelist(self):

def list_local_models(self):
"""
Dump a list of models in the local code tree that need annotations to stdout.
Dump a list of models in the local code tree that are annotation eligible to stdout.
"""
if self.local_models:
self.echo(
Expand Down Expand Up @@ -249,16 +249,16 @@ def check_coverage(self):
Returns:
Bool indicating whether or not the number of annotated models covers a percentage
of total models needing annotations greater than or equal to the configured
of total annotation eligible models greater than or equal to the configured
coverage_target.
"""
self.echo("\nModel coverage report")
self.echo("-" * 40)
self.echo("Found {total} total models.".format(**self.model_counts))
self.echo("{needing_annotation} needed annotation, {annotated} were annotated.".format(**self.model_counts))
self.echo("{annotation_eligible} were eligible for annotation, {annotated} were annotated.".format(**self.model_counts))

if self.model_counts['needing_annotation'] > 0:
pct = float(self.model_counts['annotated']) / float(self.model_counts['needing_annotation']) * 100.0
if self.model_counts['annotation_eligible'] > 0:
pct = float(self.model_counts['annotated']) / float(self.model_counts['annotation_eligible']) * 100.0
pct = round(pct, 1)
else:
pct = 100.0
Expand Down Expand Up @@ -359,27 +359,25 @@ def setup_django():
django.setup()

@staticmethod
def get_models_requiring_annotations():
def get_models_requiring_annotations(app_name=None):
"""
Determine all local and non-local models via django model introspection.
Note that non-local models returned may contain 1st party models (authored by
edX). This is a compromise in accuracy in order to simplify the generation
of this list, and also to ease the transition from zero to 100% annotations
in edX satellite repositories.
Returns:
tuple:
2-tuple where the first item is a set of local models, and the
second item is a set of non-local models.
"""
DjangoSearch.setup_django()
local_models = set()
non_local_models = set()
models_requiring_annotations = []
annotation_eligible_models = []
total_models = 0

for app in apps.get_app_configs():
if app_name and not app.name.endswith(app_name):
continue

Check warning on line 379 in code_annotations/find_django.py

View check run for this annotation

Codecov / codecov/patch

code_annotations/find_django.py#L379

Added line #L379 was not covered by tests

for root_model in app.get_models():
total_models += 1
if DjangoSearch.requires_annotations(root_model):
Expand All @@ -388,6 +386,6 @@ def get_models_requiring_annotations():
else:
local_models.add(root_model)

models_requiring_annotations.append(DjangoSearch.get_model_id(root_model))
annotation_eligible_models.append(DjangoSearch.get_model_id(root_model))

return local_models, non_local_models, total_models, models_requiring_annotations
return local_models, non_local_models, total_models, annotation_eligible_models
4 changes: 2 additions & 2 deletions tests/test_django_generate_safelist.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def test_seeding_safelist(local_models, non_local_models, **kwargs):
local_models,
non_local_models,
0, # Number of total models found, irrelevant here
[] # List of model ids that need anntations, irrelevant here
set() # List of model ids that are eligible for annotation, irrelevant here
)

def test_safelist_callback():
Expand Down Expand Up @@ -73,7 +73,7 @@ def test_safelist_exists(**kwargs):
Test the success case for seeding the safelist.
"""
mock_get_models_requiring_annotations = kwargs['get_models_requiring_annotations']
mock_get_models_requiring_annotations.return_value = ([], [], 0, [])
mock_get_models_requiring_annotations.return_value = (set(), set(), 0, [])

result = call_script_isolated(
['django_find_annotations', '--config_file', 'test_config.yml', '--seed_safelist']
Expand Down
17 changes: 17 additions & 0 deletions tests/test_find_django.py
Original file line number Diff line number Diff line change
Expand Up @@ -472,3 +472,20 @@ def test_setup_django(mock_django_setup):
"""
mock_django_setup.return_value = True
DjangoSearch.setup_django()


@patch.multiple(
'code_annotations.find_django.DjangoSearch',
get_models_requiring_annotations=DEFAULT
)
def test_find_django_no_action(**kwargs):
"""
Test that we fail when there is no action specified.
"""

result = call_script_isolated(
['django_find_annotations', '--config_file', 'test_config.yml'],
)

assert result.exit_code == EXIT_CODE_FAILURE
assert 'No actions specified' in result.output

0 comments on commit 20dc66d

Please sign in to comment.