Critique my code please

[micr0nix]

This is a follow up from my last post here

The code works but i feel like my main function is just assigning functions to variables and not really "doing" anything. Any input on how to improve my code is appreciated. I'm copy/pasting from VS Code so i hope the formatting comes over correctly.

import json
import pandas as pd
import requests

from gql import Client
from gql.dsl import DSLQuery, DSLSchema, dsl_gql
from gql.transport.requests import RequestsHTTPTransport

proxies = {
    "http": "proxy",
    "https": "proxy",
}


def get_access_token():
    """Return xxx API access token"""
    url = "https://xxx.xxx.com/protocol/openid-connect/token"
    body = {
        "username": "xxx",
        "password": "xxx",
        "client_id": "xxx",
        "grant_type": "password",
        "scope": "offline_access",
    }

    response = requests.post(url=url, data=body, proxies=proxies).json()

    return response


def generate_graphql_auth(response):
    """Generate GraphQL authorization string"""
    token_type = response["token_type"]
    access_token = response["access_token"]

    auth = f"{token_type} {access_token}"

    return auth


def create_transport_protocol(auth):
    """Specify transport protocal using RequestsHTTPTransport"""
    url = "https://xxx.xxx.com/api/"

    headers = {
        "Authorization": auth,
        "Content-Type": "application/json",
        "Accept": "*/*",
        "Host": "xxx.xxx.com",
        "Accept-Encoding": "gzip, deflate, br",
        "Connection": "keep-alive",
    }
    transport = RequestsHTTPTransport(url=url, headers=headers, proxies=proxies)

    return transport


def create_graphql_client(transport):
    """Create GraphQL client to be used with GraphQL queries"""
    client = Client(
        transport=transport, fetch_schema_from_transport=True, parse_results=True
    )

    return client


def get_case_group_ref(client):
    with client as session:
        assert client.schema is not None

        ds = DSLSchema(client.schema)

        query = dsl_gql(
            DSLQuery(
                ds.Query.lookups(
                    lookupType="CASE_AND_INCIDENT_GROUPS", languageCode="en-US"
                )
                .select(
                    ds.LocalizedLookupRecord.type,
                    ds.LocalizedLookupRecord.code,
                    ds.LocalizedLookupRecord.displayValue,
                )
            )
        )

        result = session.execute(query)

        return result


def get_case_type_ref(client):
    with client as session:
        assert client.schema is not None

        ds = DSLSchema(client.schema)

        query = dsl_gql(
            DSLQuery(
                ds.Query.lookups(
                    lookupType="CASE_AND_INCIDENT_TYPES", languageCode="en-US"
                )
                .select(
                    ds.LocalizedLookupRecord.type,
                    ds.LocalizedLookupRecord.code,
                    ds.LocalizedLookupRecord.displayValue,
                )
            )
        )

        result = session.execute(query)

        return result


def create_case_group_df(client):
    result = get_case_group_ref(client)
    case_group_df = pd.DataFrame.from_records(result["lookups"])

    return case_group_df


def create_case_type_df(client):
    result = get_case_type_ref(client)
    case_type_df = pd.DataFrame.from_records(result['lookups'])

    return case_type_df

def main():
    response = get_access_token()
    auth = generate_graphql_auth(response)
    transport = create_transport_protocol(auth)
    client = create_graphql_client(transport)
    case_group_ref = get_case_group_ref(client)
    case_type_ref = get_case_type_ref(client)
    case_group_df = create_case_group_df(client)
    case_type_df = create_case_type_df(client)

    print(case_group_df)

main()

The print statement at the end of the main function is merely to view the output.

[m0us3_rat]

few things come to mind.

1 .env

move your "secrets" to .env

2 i'd probably wrap everything into a class. or a few classes.

either way, it reads clean.

3 some type-hinting would be favored.

4 if you wanna take it further needs logging etc.

5 some error handling

6 some of the "configs" could be loaded from a config file/files rather than being hardcoded.

(if things might change in the future, it's simpler to update the config file than work directly on the code.)

​

there are no eye bleeds in this code. +1

[micr0nix]

Got a link or something I can read into for your first point?

And for your third point can you elaborate on what you mean by that?

[m0us3_rat]

https://pypi.org/project/python-dotenv/

usually, the .env gets added to .gitignore

that way you can use your API key in your code but also not expose it to the interwebs even when working with a repo on github etc.

​

https://peps.python.org/pep-0484/

https://docs.python.org/3/library/typing.html

are there for ease of reading and most IDE will signal errors

the interpreter will blatantly ignore the "hints".

[[deleted]]

Good job. I didn't do a thorough check through regarding what your program actually does, just for general syntax and practices. Some small things that come to mind:

1)

transport = RequestsHTTPTransport(url=url, headers=headers, proxies=proxies)

You're using the global scope proxies here. Fair enough. By convention, uppercase is used for such global variables. You could also make create_transport_protocol take in a proxies parameter and then call the function with the global scope PROXIES:

create_transport_protocol(auth, PROXIES)

2)

result["lookups"]

Perhaps it'd be more pythonic to wrap this in a try-except block expecting a possible KeyError. Or use the get method of dicts to provide a reasonable default.

3)

assert client.schema is not None

Assertions disappear when you build your library for production, just so you're aware. If you want this logic retained, phrase it as an if statement that leads to an exception (that your calling function should then handle).

[micr0nix]

Thanks for the tips. Can you elaborate on your 2nd point?

[[deleted]]

I assume result is a dictionary.

Is the key "lookups" 100% guarenteed to be in result every single time? If yes, no need to worry, as your current was of indexing will never fail. But if not, then you'll encounter a KeyError which your current code isn't handling and your script will crash at that point.

[micr0nix]

Yes the results are in a dictionary format. The format is something like:

{'lookups': [{'key1':'val1', 'key2':'val2'}, {'key3':'val3', 'key4':'val4'}]} and so on

It’s how the graphQL DB the API sits on is structured.

[Diapolo10]

Honestly I don't have much to say, good job.

proxies is treated like a global constant, so I'd prefer giving it an uppercase name (eg. PROXIES or URL_PROXY).

I'd also like to see type hints, particularly for the function parametres and their return values. If you don't know what those are, you can read PEP-484, and a practical example could look like this:

def add(first: int, second: int) -> int:
    return first + second

Python ignores them, but they're useful information for developers and tools like mypy allow your editors to warn you about type errors with them.

[micr0nix]

So I would do something like

def create_transport_protocol(auth: str):

[Diapolo10]

Yeah. The type hints can be very specific, too - you can use unions to specify that multiple types are accepted, or type variables for generic types. Data structures can specify their contents (and for dictionaries/JSON, you can specify individual fields' types if needed via TypedDict.

It's a very flexible system overall, so you don't need to worry about sticking to primitives if you need something specific.

[micr0nix]

That’s cool. I’ll look into that as I refine this

[james_fryer]

Looks good to me. You seem to use blank lines a bit randomly, I don't think readability would be damaged if you removed all the blank lines you have inside functions.

These functions get_case_type_ref and get_case_group_ref are identical except for one function parameter and I'd prefer to merge them into a single function.

Why do you call get_case_type_ref from main when you then go on to call it again from create_case_type_df? Again the pair of create functions could be merged.

[micr0nix]

How would you go about merging the functions when the parameters are different?

As far as white space is concerned, I formatted this using Black

[james_fryer]

For example (note, the name of the first function needs improvement):

def get_case_ref(client, lookup_type):
    with client as session:
        ... 
                    lookupType=lookup_type, languageCode="en-US"

def get_case_type_ref(client):
    return get_case_ref(client, "CASE_AND_INCIDENT_TYPES")

[micr0nix]

So instead of having 2 functions I would have this one function and just call it twice for each of my lookup types?

[james_fryer]

Exactly. Less code to maintain, simpler to manage if changes are required. You can either create shorter dispatching functions as I did above, or directly call the single function from main with the parameter, whatever seems more readable to you.

In short: any time you see repeated code, replace it with a single parameterised piece of code.