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

Leave location validation to specific carbon API #56

Merged
merged 8 commits into from
Aug 1, 2023
Merged

Conversation

tlestang
Copy link
Collaborator

Currently the user-specified location (postcode for carbonintensity.org.uk) is validated in a separate function check_clean_arguments.validate_location

def validate_location(location, choice_CI_API):
    if choice_CI_API == 'carbonintensity.org.uk':
        # in case the long format of the postcode is provided:
        loc_cleaned = location.split()[0]
    # ...

However location format (or whether or not location data is relevant at all) is specific to the API supplying the carbon forecast. This PR moves location validation logic to the API-specific functions. In the case of carbonintensity.org.uk, this is ciuk_parse_response_data. API specific parsing functions in CI_api_interface module are responsible for raising an InvalidLocationError if the supplied location is deemed invalid.

Furthermore -- and this might be specific to carbonintensity.org.uk -- postcode is validated a posteriori based on the response from the API.

A few examples

$ cats -d180 -l O
Error: unknown location O

$ cats -d180 -l OXFDG
Error: unknown location OXFDG

$ cats -d180 -l 'SW7 EAZ' # 2-part postcode with a space is valid (only first part is used).
Best job start time: 2023-07-27 01:00:00

$ cats -d180 -l 'SW7EAZ' # 2-part postcode without a space is invalid.
Error: unknown location SW7EAZ

@ljcolling
Copy link
Contributor

Just a comment on the postcode validation... the API only needs the outward code, and not the inward code part of the postcode. The outward part varies in length, but the inward part is (almost) always 3 characters, so if the postcode is more than 4 chars then dropping the last 3 might make it valid (this is just a suggestion, but it could make things too messy because it involves doing some validation locally and not on the API)

Also, it might be nice to have a more informative error message for the invalid location just highlighting the format it should be in.

@ljcolling
Copy link
Contributor

Yeah, that looks good 🚀

@tlestang
Copy link
Collaborator Author

tlestang commented Aug 1, 2023

Just added a small check based on your first suggestion. Not a silver bullet, but should make most cases work (unless typo).

@tlestang tlestang merged commit ce0a510 into main Aug 1, 2023
3 checks passed
@tlestang tlestang deleted the validate_location branch August 1, 2023 15:00
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.

2 participants