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

New feature - Translate natural geographic level naming to API abbreviations on query input #70

Merged
merged 7 commits into from
Jan 31, 2025

Conversation

rmbielby
Copy link
Contributor

Brief overview of changes

I've added a draft look-up table for translating natural geographic level names (e.g. National) to the API versions (e.g. NAT) and I've applied a screening on the query input to translate from the former to the latter.

This lays some groundwork for translations at the other end on the query results, but saving that for later.

Why are these changes being made?

The API is quite prescriptive when it comes to providing geographic levels and this is intended to give users a more flexible / forgiving experience.

Detailed description of changes

So that I'm not repeating code, I've stuck this in todf_geographies(), which feeds into post_dataset() and get_dataset()

Additional information for reviewers

Issue ticket number/s and link

Partial groundwork for some of #5

@rmbielby rmbielby self-assigned this Jan 30, 2025
@rmbielby rmbielby added new feature New feature or request geographies labels Jan 30, 2025
@rmbielby rmbielby added this to the Mop up sweep milestone Jan 30, 2025
Copy link
Contributor

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

Tests themselves all pass, but there's an issue with the vignette that you need to fix (is causing all the CI failures)

Slightly surprised that this was the bit you did, would have thought having National in the output would be more helpful than in the inputs for the query, though happy for this to go in and to do the other half later (once the vignette is fixed)

@rmbielby
Copy link
Contributor Author

Tests themselves all pass, but there's an issue with the vignette that you need to fix (is causing all the CI failures)

Slightly surprised that this was the bit you did, would have thought having National in the output would be more helpful than in the inputs for the query, though happy for this to go in and to do the other half later (once the vignette is fixed)

I'm just working through using eesyapi for the attendance dashboard, so dealing with things in the order I hit them in the implementation. This was prompted by linking the API call to the inputs, which meant having to write something in the app or eesyapi to convert National / Regional / Local authority to NAT / REG / LA. More efficient to do just it in eesyapi now, rather than in the app now and then eesyapi later down the line.

Copy link
Contributor

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

One of my tests is failing locally but I think that's just an EES dev issue.

There's one tiny typo I've spotted and made a suggestion for though happy to approve and let you sort that one out as you merge it in

@rmbielby rmbielby merged commit cc85c11 into main Jan 31, 2025
1 of 7 checks passed
@rmbielby rmbielby deleted the new-feature/geographic_level-humanVapi branch January 31, 2025 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geographies new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants