Skip to content

More cleanup of load.py#8

Open
pamelafox wants to merge 1 commit intomasterfrom
load-cleanup
Open

More cleanup of load.py#8
pamelafox wants to merge 1 commit intomasterfrom
load-cleanup

Conversation

@pamelafox
Copy link
Contributor

No description provided.

PARSONS_FOLDER_NAME = 'parsons_probs'
PARSONS_FOLDER_PATH = './parsons_probs'
PARSONS_GLOB = 'parsons_probs/*.py'
PROBLEM_PATHS = [PARSONS_FOLDER_PATH]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given this is always a list with one constant, the added complexity of its existence doesn't seem warranted.

for path in PROBLEM_PATHS:
config_files.append(os.path.join(os.path.abspath(path), problem_name.lower() + ".yaml"))
return load_config_file(config_files)
path = os.path.join(os.path.abspath(PARSONS_FOLDER_PATH), problem_name.lower() + ".yaml")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the recognition that there is only one path, this logic is shorter, so I combined it with the function above.



def problem_name_to_file(problem_name, extension):
return f'{PARSONS_FOLDER_PATH}/{problem_name.lower()}.{extension}'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear to me why we use os.path.join above but just an f-string here. Is there a reason? Otherwise I'd like to standardize on one way of creating paths. I imagine os.path.join might take care of weird slash issues or some such.

submitted_code = request.form['submitted_code']
parsons_repr_code = request.form['parsons_repr_code']
fname = f'{PARSONS_FOLDER_PATH}/{problem_name.lower()}.py'
fname = problem_name_to_file(problem_name, 'py')
Copy link
Contributor Author

@pamelafox pamelafox Feb 26, 2022

Choose a reason for hiding this comment

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

I moved logic into the load file for testability (since this file is so hard to test right now, given the complexity of the okclient import).

with self.assertRaises(Exception) as context:
load.load_config('remove_indexes')
self.assertIn('remove_indexes.yaml', str(context.exception))
load.load_config("remove_indexes")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran the black formatter on this file so there are some whitespace/quote changes.

self.assertIn('smartfridge.yaml', str(context.exception))
self.assertIn("smartfridge.yaml", str(context.exception))

def test_problem_name_to_file(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual change is the addition of this test function.

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.

1 participant