3
\$\begingroup\$

I wrote this first function that use a JSON datafile as an input

def jsonfile_loader_work_rate(filename):
    '''Load the json file, to retrieve the interventions id and the cars id
    '''
    try:
        with open(filename, "r") as jsonfile:
            json_data = json.load(jsonfile)
        cars = [
            json_data["cars"][val]["id"]
            for val in range(len(json_data["cars"]))]
        interventions = [
            json_data["interventions"][val]["id"]
            for val in range(len(json_data["interventions"]))
        ]
        return cars,interventions
    except:
        print('No date or no file')

This is the json file

{
  "interventions": [
    {
      "id": 1, "name": "batteria cambio",
      "parts_spec": [
        {"type": "battery", "count": 1}
      ]
    },
    {
      "id": 2, "name": "Cambio de lo disc y de la plakette",
      "parts_spec": [
        {"type": "front_brake_pad", "count": 2},
        {"type": "front_brake_disc", "count": 2}
      ]
    }
  ],
  "cars": [
    { "id": 1, "manufacturer": "Peugeot", "model": "307 CC", "version": "2.0 16v 140cv"},
    { "id": 2, "manufacturer": "BMW", "model": "Série 3", "version": "318i 136cv"},
    { "id": 3, "manufacturer": "Toyota", "model": "Rav 4 3", "version": "2.2 D-4D 4WD 136cv"}
  ],
  "parts": [
    {"id": 1, "type": "battery", "car_id": 1, "price": "95.99"},
    {"id": 10, "type": "battery", "car_id": 1, "price": "106.00"},
    {"id": 11, "type": "battery", "car_id": 1, "price": "84.40"},
    {"id": 12, "type": "battery", "car_id": 2, "price": "121.00"},
    {"id": 13, "type": "battery", "car_id": 2, "price": "150.05"},
    {"id": 4, "type": "battery", "car_id": 2, "price": "123.10"},
    {"id": 14, "type": "battery", "car_id": 3, "price": "109.00"},
    {"id": 7, "type": "battery", "car_id": 3, "price": "97.99"},
    {"id": 14, "type": "battery", "car_id": 3, "price": "200.00"},
    {"id": 2, "type": "front_brake_pad", "car_id": 1, "price": "17.20"},
    {"id": 3, "type": "front_brake_disc", "car_id": 1, "price": "80.00"},
    {"id": 5, "type": "front_brake_pad", "car_id": 2, "price": "24.5"},
    {"id": 6, "type": "front_brake_disc", "car_id": 2, "price": "110.4"},
    {"id": 8, "type": "front_brake_pad", "car_id": 3, "price": "30.34"},
    {"id": 9, "type": "front_brake_disc", "car_id": 3, "price": "120.50"}
  ],
  "workshops": [
    {"id": 1, "name": "XYZ", "hourly_rate": "67.00", "preferred_part_price": "median"},
    {"id": 2, "name": "ZYX", "hourly_rate": "72.99", "preferred_part_price": "most_expensive"},
    {"id": 3, "name": "YZX", "hourly_rate": "63.50", "preferred_part_price": "cheapest"}
  ]
}
    

When I'm running jsonfile_loader_work_rate('data.json'), it gives me the following result ([1, 2, 3], [1, 2]).

I'm then using the output of the function, in another function

def retrieve_with_api_url_labour_time(loader_cars_interv,url):
    cars, interventions = loader_cars_interv
    spent_time_by_op_results = []
    for val_car in cars:
        for val_interv in interventions:
            results = requests.get(f"{url}/{val_car}/{val_interv}")
            time_load = json.loads(results.text)
            time_load = time_load["labourTime"]
            spent_time_by_op_results.append(
                                            {"car_id": val_car,
                                             "interv_id": val_interv,
                                             "time_spent": time_load}
            )
    return spent_time_by_op_results

I cannot give you the URL as I don't want to display it publicly.

However, I can give you a sample of the results when I query the API

{"labourTime":"00:20:00"}

Below is the final desired result. My two functions, imbricated, provides it.

[{'car_id': 1, 'interv_id': 1, 'time_spent': '00:20:00'}, {'car_id': 1, 'interv_id': 2, 'time_spent': '01:10:00'}, {'car_id': 2, 'interv_id': 1, 'time_spent': '00:25:00'}, {'car_id': 2, 'interv_id': 2, 'time_spent': '01:40:00'}, {'car_id': 3, 'interv_id': 1, 'time_spent': '00:29:00'}, {'car_id': 3, 'interv_id': 2, 'time_spent': '01:55:00'}]

I run the two functions like that

print(retrieve_with_api_url_labour_time(jsonfile_loader_work_rate('data.json'),'https://www.acme.ie/apipint'))

The question is the following: Is there another way to run the script retrieve_with_api_url_labour_time, instead of what I'm doing?

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Use PEP484 type hints.

Don't use range(len()) on your data; iterate over the inner dictionaries directly.

Never bare except; let exceptions fall through so that they can be properly handled at the outer level. If you do want to translate internal exception traces to user-friendly messages, you can; but the way shown in the original post is not the way to do it. Among other reasons,

  • The caller will not know that anything failed until it tries to use the return value and finds out - surprise - that it's None.
  • A user-break (Ctrl+C) issued during the execution of that function will be swallowed by your except:, when it shouldn't be.
  • Which is it: no data, no file, or something else? The message is both not specific enough and not accurate enough. If you handle the exception at the outer level, you should have separate except clauses covering the specific failure being described.

Whenever you call requests, make sure to check for failure - which it doesn't by default.

Don't call json.loads on a Requests response; use its own .json() method instead.

Don't use dictionaries for internal data representation; use class instances - dataclass or NamedTuple being two reasonable options.

Don't use a string type for your time_spent when it's actually a timedelta. Incredibly, Python timedelta parsing is pretty garbage and so some simple manual parsing something like I've shown will be necessary.

Consider using generators instead of building up lists for your API function.

It's fine to return two iterables from your loader, but they should be unpacked to two separate variables instead of passed around together.

Suggested

import json
from datetime import timedelta
from typing import Iterable, Collection, Iterator, NamedTuple

import requests as requests


def jsonfile_loader_work_rate(filename: str) -> tuple[
    list[int],  # car IDs
    list[int],  # intervention IDs
]:
    """Load the json file, to retrieve the interventions id and the cars id
    """

    with open(filename, 'r') as jsonfile:
        json_data = json.load(jsonfile)

    cars = [
        car['id']
        for car in json_data['cars']
    ]
    interventions = [
        intervention['id']
        for intervention in json_data['interventions']
    ]
    return cars, interventions


class LabourTime(NamedTuple):
    car_id: int
    intervention_id: int
    time_spent: timedelta


IMAGINARY = True


def parse_timedelta(text: str) -> timedelta:
    h, m, s = (int(t) for t in text.split(':'))
    return timedelta(hours=h, minutes=m, seconds=s)


def retrieve_with_api_url_labour_time(
    car_ids: Iterable[int],
    intervention_ids: Collection[int],
    url: str,
) -> Iterator[LabourTime]:
    for car_id in car_ids:
        for interv_id in intervention_ids:
            if IMAGINARY:
                time_load = {'labourTime': '25:20:01'}
            else:
                with requests.get(f'{url}/{car_id}/{interv_id}') as resp:
                    resp.raise_for_status()
                    time_load = resp.json()

            yield LabourTime(
                car_id=car_id,
                intervention_id=interv_id,
                time_spent=parse_timedelta(time_load['labourTime']),
            )


def main() -> None:
    car_ids, intervention_ids = jsonfile_loader_work_rate('273228.json')
    for labour_time in retrieve_with_api_url_labour_time(
        car_ids=car_ids,
        intervention_ids=intervention_ids,
        url='https://www.acme.ie/apipint',
    ):
        print(labour_time)


if __name__ == '__main__':
    main()
\$\endgroup\$
4
  • \$\begingroup\$ I'd suggest (changing the signature and) returning a value from main for exit codes, and then modifying the call in if __name__ == '__main__': to sys.exit(main()); then you can return non-zero values if exceptions are raised. \$\endgroup\$
    – ades
    Jan 22 at 10:10
  • \$\begingroup\$ @ades that's only really important for programs that have a non-trivial command-line interface with arguments, etc. That isn't the case here. Also note that that already happens for default exception handling: try for yourself, python -c 'j = 0; j /= j'; echo $? \$\endgroup\$
    – Reinderien
    Jan 22 at 14:55
  • \$\begingroup\$ I suggest it out of two reasons: first you can see in the OP that he wants to catch these errors (FileNotFoundError and something relating to time), and second I find a main() -> None entry kind of non-standard. The second point is more subjective, but I find it strange to change OPs code to let the errors/exceptions slip by to the user without telling OP what your rationale is for that change. \$\endgroup\$
    – ades
    Jan 22 at 15:03
  • \$\begingroup\$ @ades That there wasn't enough rationale is fair (edited); for everything else - it's off-topic and can land in chat.stackexchange.com/rooms/133429/… \$\endgroup\$
    – Reinderien
    Jan 22 at 15:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.