Skip to content

Conversation

@kellynm
Copy link
Contributor

@kellynm kellynm commented Mar 7, 2024

Created a new file-based consignment type that gets the item_per_box value from each record in the input file, allowing packaging to vary. Added sample data for variable packaging consignments and AQIM consignments. Used data swapping and commodity/port name alteration to anonymize sample data.

@kellynm kellynm requested a review from wenzeslaus March 8, 2024 16:15
@kellynm kellynm marked this pull request as ready for review March 8, 2024 16:15
Comment on lines -358 to +420
f"Unknown consignment generation method: {generation_method}"
f"Unknown consignment generation method: {generation_method} or "
f"input file type: {config['input_file']['file_type']}"
Copy link
Member

Choose a reason for hiding this comment

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

This will fail for unknown generation_method (e.g., param_based) and config['input_file']['file_type'] not provided. One way around it would be to extract config["input_file"]["file_type"] ahead of time if available and use None otherwise. This will make the code more robust overall because the case of generation_method == "input_file" and forgotten config["input_file"]["file_type"] is not handled at all.

filename=config["input_file"]["file_name"],
)
elif (generation_method == "input_file") and (
config["input_file"]["file_type"] == "variable packaging"
Copy link
Member

Choose a reason for hiding this comment

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

We use parameter_based for method and variable packaging here. It is just for consistency since it is key. input_file is both value and key, so it is input_file in both cases. I don't have a strong opinion, though. In PoPS Core, we ended up supporting all different spellings, but implementing it here would mean some extra work.

Comment on lines +337 to +345
items_per_box = int(record["ITEMS_PER_PACKAGE"])
unit = record["QUANTITY_UNIT"]

# Generate items based on quantity in records.
# Quantity unit must be stems/items.
if unit in ["Stems", "Items"]:
num_items = int(record["QUANTITY"])
else:
raise RuntimeError(f"Unsupported quantity unit: {unit}")
Copy link
Member

Choose a reason for hiding this comment

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

Can you please describe here, in the class docstring, or the PR description on the difference between this class and AQIM?

I can see the two classes are similar and we can resolve some de-duplication some other time, but it would be good to have clear specification of what are the differences.

@kellynm
Copy link
Contributor Author

kellynm commented Mar 12, 2024

Thanks for this review, @wenzeslaus! I'll get these changes incorporated shortly.

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.

3 participants