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

More cleanup of load.py #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions parsons/constants.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
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.

PARSONS_OUTFILE = f"{PARSONS_FOLDER_PATH}/test_log"
PARSONS_CORRECTNESS = f"{PARSONS_FOLDER_PATH}/correctness"
PARSONS_REPR_SUFFIX = "_repr"
Expand All @@ -12,4 +11,4 @@
REQUIRED_PROBLEMS = "required_problem_names"
OPTIONAL_PROBLEMS = "optional_problem_names"
PROBLEM_NAMES = "problem_names"
MAX_NUM_RETRIES = 20
MAX_NUM_RETRIES = 20
43 changes: 15 additions & 28 deletions parsons/load.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,12 @@

import yaml

from constants import PROBLEM_PATHS, UTILITY_FILES, PARSONS_GLOB, PARSONS_FOLDER_PATH
from constants import UTILITY_FILES, PARSONS_GLOB, PARSONS_FOLDER_PATH

def load_config_file(paths):
"""
Loads a YAML file.
Args:
paths: Either a single path or a list of paths for YAML files.

Returns: The contents of the YAML file as a defaultdict, returning None
for unspecified attributes.
"""
if type(paths) != list:
paths = [paths]
for path in paths:
try:
with open(os.path.abspath(path), 'r') as file:
config = yaml.load(file, Loader=yaml.Loader)
if type(config) == dict:
config = defaultdict(lambda: None, config)
return config
except IOError as e:
pass
raise Exception("Cannot find files {0}".format(paths))

def load_config(problem_name):
"""
Loads a YAML file, assuming that the YAML file is located at {PROBLEM_PATHS}/{problem_name}.yaml
Loads a YAML file, assuming that the YAML file is located at {PARSONS_FOLDER_PATH}/{problem_name}.yaml
Normalizes problem_name to lowercase as all filenames should be lowercased.

Args:
Expand All @@ -39,10 +18,16 @@ def load_config(problem_name):
Returns: The contents of the YAML file as a defaultdict, returning None
for unspecified attributes.
"""
config_files = []
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.

try:
with open(os.path.abspath(path), 'r') as file:
config = yaml.load(file, Loader=yaml.Loader)
if type(config) == dict:
config = defaultdict(lambda: None, config)
return config
except IOError as e:
pass
raise Exception("Cannot find path {0}".format(path))

def problem_name_from_file(filename):
with open(filename, "r", encoding="utf8") as f:
Expand All @@ -54,7 +39,9 @@ def problem_name_from_file(filename):
name = func_sig[:func_sig.index('(')]
return name


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.


def path_to_name(names_to_paths, path):
for key, val in names_to_paths.items():
if val == path:
Expand Down
6 changes: 3 additions & 3 deletions parsons/local_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from client.api.assignment import load_assignment
from client.cli.common import messages
from output import DisableStdout
from load import load_config, path_to_name, problem_name_from_file
from load import load_config, path_to_name, problem_name_from_file, problem_name_to_file
from constants import *

from multiprocessing import Semaphore
Expand Down Expand Up @@ -133,7 +133,7 @@ def submit():
problem_name = request.form['problem_name']
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).

write_parsons_prob_locally(fname, submitted_code, parsons_repr_code, True)
test_results = grade_and_backup(problem_name)
return jsonify({'test_results': test_results})
Expand Down Expand Up @@ -302,7 +302,7 @@ def get_useful_syntax_error_logs(logs, problem_name):
return logs[:traceback_index + 1] + logs[file_index:]

def count_docstring_lines(problem_name):
fname = f'{PARSONS_FOLDER_PATH}/{problem_name.lower()}.py'
fname = problem_name_to_file(problem_name, 'py')
num_lines = 0
with open(fname, "r", encoding="utf8") as f:
for i, line in enumerate(f):
Expand Down
22 changes: 16 additions & 6 deletions parsons/test_load.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,29 @@

import load

class TestLoad(unittest.TestCase):

class TestLoad(unittest.TestCase):
def test_load_config_lowercased(self):
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("remove_indexes.yaml", str(context.exception))

def test_load_config_camelcased(self):
with self.assertRaises(Exception) as context:
load.load_config('SmartFridge')
load.load_config("SmartFridge")
# Make sure it lowercases the file name
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.

self.assertEqual(
load.problem_name_to_file("smartfridge", "py"),
"./parsons_probs/smartfridge.py",
)
self.assertEqual(
load.problem_name_to_file("SmartFridge", "py"),
"./parsons_probs/smartfridge.py",
)


if __name__ == '__main__':
if __name__ == "__main__":
unittest.main()