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.

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

Add national risk data and tests #69

Merged
merged 2 commits into from
Jan 18, 2024
Merged

Add national risk data and tests #69

merged 2 commits into from
Jan 18, 2024

Conversation

b-j-mills
Copy link
Collaborator

This was the easiest way I could think of to construct the rows for the database but happy to implement any suggestions you have!

@b-j-mills b-j-mills requested a review from mcarans January 17, 2024 20:23
Copy link

github-actions bot commented Jan 17, 2024

Test Results

4 tests  ±0   4 ✅ ±0   3m 29s ⏱️ +18s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit d8f752d. ± Comparison against base commit 5a372a1.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Jan 17, 2024

Pull Request Test Coverage Report for Build 7572306362

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 93.021%

Totals Coverage Status
Change from base Build 7441436109: 0.08%
Covered Lines: 893
Relevant Lines: 960

💛 - Coveralls

if hxl_tag == "#risk+class":
value = _get_risk_class_code_from_data(value)
dict_of_dicts_add(rows, admin_code, hxl_tag, value)
for location in rows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the purpose of this additional loop (rather than placing the code in this loop where the dict_of_dicts_add line is) to try to ensure that rows are grouped by location rather than by #risk+x when you look at them in the db table?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The format of the national risk table is wide rather than long, unlike the other tables (example), so I'm essentially transposing the results dictionary so it's location based instead of hxl tag based. The only other way I could think of to do that was using map to pull out the values of each dictionary for each location, but it was more difficult to understand when I read it back:
hxl_tags = admin_results["headers"][1]
locations = list(admin_results["values"][0].keys())
for location in locations:
values = list(map(lambda x: x[location], admin_results["values"]))
values = {hxl_tag: value for hxl_tag in hxl_tags for value in values}

Copy link
Contributor

@mcarans mcarans left a comment

Choose a reason for hiding this comment

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

I've approved. I made a comment about the additional loop for you to look at.

@b-j-mills
Copy link
Collaborator Author

I looked at it again and found a way to simplify and avoid the second loop! I'll go ahead and merge.

@b-j-mills b-j-mills merged commit f3b629d into main Jan 18, 2024
3 checks passed
@turnerm turnerm deleted the HAPI-136 branch May 27, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants