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

trying to rewrite my R function in python #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

milanwiedemann
Copy link
Member

just an example function that writes SQL using python. although the code works it's slightly out of date and I mainly started this to practice my python skills.

scripts/sql-ing-lookup.py Show resolved Hide resolved
sql_query_start_l3 = "INNER JOIN dmd.ing as ing ON ing.id = vpi.ing\n"
sql_query_start_l4 = "LEFT JOIN dmd.ddd on vmp.id = ddd.vpid\n"

sql_query_start = sql_query_start_l1 + sql_query_start_l2 + sql_query_start_l3 + sql_query_start_l4
Copy link
Contributor

Choose a reason for hiding this comment

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

why 4 strings joined rather than 1 string?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. The names of these variables don't help me understand why they are different. If they are, then also consider using an f-string:

first_name = "Dizzee"
last_name = "Rascal"
full_name = f"{first_name} {last_name}"  # "Dizzee Rascal"

Choose a reason for hiding this comment

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

Given none of that is parameterised I'd be inclined to write it as a triple-quoted multiline string

sql_query_start = """
SELECT 
  'vmp' AS type,
  CAST(vmp.id AS STRING) AS id,
  bnf_code,
  vmp.nm,
  ing.nm AS ingredient,
  ddd.ddd 
FROM dmd.vmp"

ingredient_list = [f"{ingredient}%" for ingredient in ingredient_list]
elif wildcards == "both":
ingredient_list = [f"%{ingredient}%" for ingredient in ingredient_list]
elif wildcards == "none":
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have the default as None rather than a string and then no need to do anything here

scripts/sql-ing-lookup.py Show resolved Hide resolved
return sql_query_return


#%%
Copy link
Member

Choose a reason for hiding this comment

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

What's this for?

Choose a reason for hiding this comment

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

sql_query_where = "WHERE " + " OR ".join([f"{sql_str_lower_01}ing.nm{sql_str_lower_02} LIKE {ing}" for ing in ingredient_list] ?

@@ -0,0 +1,80 @@
#%%
# Define function to query details for ingredients
Copy link
Member

Choose a reason for hiding this comment

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

Could this be part of the docstring?

sql_query_where = []
sql_query_or = list()

# For loop writing WHERE and OR statements
Copy link
Contributor

Choose a reason for hiding this comment

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

If no wildcards, could use SQL IN here. To do this I normally convert list to tuple and then use an f-string, e.g.

# convert list to tuple for use in SQL query
names_tuple = tuple(names)
if len(names_tuple)==1:
    # remove comma if only one item
    names_tuple = str(names_tuple).replace(",","")

sql = f'WHERE ingredient IN {names_tuple}'

Choose a reason for hiding this comment

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

or ",".join(ingredient_list) handles the case where there is only one item in a list more concisely.

raise ValueError('Second argument "wildcards" should be one of: "prefix", "suffix", "both", "none')

# Define empty objects for loop
sql_query_where = []
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will just be a string so no need to define it here first?

@Jongmassey
Copy link

Underscores rather than hyphens in the filename would make it more consistent with the other scripts

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.

None yet

4 participants