-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/automated pitt pax v2 #7
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fit for purpose, but a few comments on more style than substance.
@@ -17,6 +17,34 @@ | |||
import botocore | |||
from concurrent.futures import ThreadPoolExecutor, as_completed | |||
|
|||
#boolean to decide whether script will be automatic or not | |||
isAutomated = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why omit the global
keyword from isAutomated
, but present it on first_step
, et. al.? I don't think the global
keyword is actually doing anything on these variables outside of a function scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I had issues running the automated scripts when using the three parameters, since it would cause a variable not declared error. That's why i originally used the global descriptor for the three, but like you said I don't think I need to it might just be a scope issue for one of them?
first_step = sys.argv[1] | ||
c_f_input = sys.argv[2] | ||
send_to_s3 = sys.argv[3] | ||
isAutomated = checkParameters(first_step, c_f_input, send_to_s3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some interesting side effects introduced here.
The user input for first_step
, et.al., is retained even if checkParameters()
determines it to be invalid. You consider this for each subsequent usage by using a if
conditional built on isAutomated
, but will each future developer be similarly aware of this side effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe doing something within the checkParameters
method that doesn't retain the variables if it's false?
@@ -194,7 +222,7 @@ def fCreateOpexFragment(list_folders_in_dir, list_files_in_dir, LegacyXIP, | |||
desc_metadata = ET.SubElement(opex_root, opex + 'DescriptiveMetadata') | |||
if LegacyXIP != "": | |||
LegacyXIP = ET.SubElement(desc_metadata, 'LegacyXIP', {'xmlns': 'http://preservica.com/LegacyXIP'}) | |||
access_ref = ET.SubElement(legacyXIP, 'AccessionRef') | |||
access_ref = ET.SubElement(LegacyXIP, 'AccessionRef') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This drive-by correction might be better as part of its own pull request, with its own description.
automation option for pitt_pax_v2.py. Script still interactively if incorrect command line arguments are passed.