Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always return a value from _update_or_create_domain_metrics #35606

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −0
Open

Always return a value from _update_or_create_domain_metrics #35606

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions corehq/apps/data_analytics/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ def _update_or_create_domain_metrics(domain, all_stats):
notify_exception(
None, message='Failed to create or update domain metrics for {domain}: {}'.format(e, domain=domain)
)
return None, False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I'd move this out of the except block, it'd be a tiny bit more robust to future updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What updates are you thinking this would help with? I'm imagining maybe if the except block is changed in the future to catch a more specific exception?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was imagining the return statement gets moved out of the try...although that would presumably mean that it gets moved below the whole try/except, so then you would still want the return None, False to be inside the except.

Really, it just visually catches my attention when a function doesn't end with a return statement on the left-most level of indentation as the function body, and it'll catch my attention again if I read/modify/review this code in the future. But then I read the function and see that it will always return something.

Copy link
Contributor Author

@nospame nospame Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Would it be clearer/more conventional to always assign metrics, created (even if we're not using created in practice, that's still what the value is) and return those two values?

Rethought this comment immediately after writing it. Since we're never using created, this can just return the metrics, and update the the update_domain_metrics_for_domains to only expect a single value to be returned. Something like:

def update_domain_metrics_for_domains(domains):
    ...
    for domain in domains:
            metrics = _update_or_create_domain_metrics(domain, all_stats)
    ...


def _update_or_create_domain_metrics(domain, all_stats)
    try:
        ...
        metrics, __ = DomainMetrics.objects.update_or_create(
            defaults=metrics_dict,
            domain=domain_obj.name,
        )
    except Exception as e:
        notify_exception(
            None, message='Failed to create or update domain metrics for {domain}: {}'.format(e, domain=domain)
        )
        metrics = None

    return metrics

What do you think?



def get_domains_to_update():
Expand Down
Loading