Skip to content
This repository was archived by the owner on Jun 15, 2024. It is now read-only.

Conversation

@CVilla17
Copy link
Contributor

No description provided.

@CVilla17 CVilla17 requested review from bearmit and nh916 March 27, 2023 19:42
Copy link

@bearmit bearmit left a comment

Choose a reason for hiding this comment

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

good job!

if isinstance(parsed_cell, dict):
cell_type = parsed_cell["type"]
cell_key = parsed_cell["key"]
cell_value = parsed_cell["value"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend stripping it of white spaces as well

elif cell_type == "identifier":
if cell_key == "Experiment or Inventory":
if cell_value == "I":
if cell_value.lower() == "i":
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 is fine, but if we wanted to make the code shorter we could do

inventory = cell_value.lower() == "i"

group-obj
returns-tuple of dicts of objs
group-cript Group node
returns-tuple of dicts of reference nodes and citation nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

good docstrings!

elif cell_type == "property":
if parsed_cell["key"] == "use_existing":
use_existing = cellToBool(parsed_cell["value"])
use_existing = is_cell_true(parsed_cell["value"])
Copy link
Contributor

Choose a reason for hiding this comment

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

beautiful!

# try to get the material using its name
name_ = parsed_material["name"]["value"]
newProject = cript.Project.get(
mat_name = parsed_material["name"]["value"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not have known what mat_name if it wasn't for the comment. How come we didn't go with material_name ?

return str(val).lower() != "false"


def copyMaterial(material, new_project, project):
Copy link
Contributor

Choose a reason for hiding this comment

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

beautiful function!
✅ great docstrings!
✅ great comments!
✅ clear self documenting code!

great job all around!

The only thing that I could think to make it even better would be if we had python typing and docstrings in either Google or Pep style

@nh916 nh916 merged commit ac1589a into develop Mar 29, 2023
@nh916 nh916 deleted the feature_clean_updates branch March 29, 2023 23:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants