GreenScheduler / cats

CATS: the Climate-Aware Task Scheduler :cat2: :tiger2: :leopard:
https://greenscheduler.github.io/cats/
MIT License
50 stars 8 forks source link

Leave location validation to specific carbon API #56

Closed tlestang closed 1 year ago

tlestang commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Yeah, that looks good :rocket:

tlestang commented 1 year ago

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