Skip to content

Conversation

cpsievert
Copy link
Collaborator

Closes #860

if not isinstance(body, Tag) or body.name != "body":
raise ValueError("Expected a <body> tag")

as_fillable_container(body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is being called for its side effect, modifying the object in place. I think in all other instances in the code base, we call it for its return value.

The function definition has a question of how it should work:

@add_example()
# TODO-future-API; These methods mutate their input values. Should they return a new object instead? Or not return at all (like `LIST.sort()`)?
def as_fillable_container(
tag: TagT,
) -> TagT:
tag_prepend_class(tag, FILL_CONTAINER_CLASS)
tag.append(fill_dependency())
return tag

I think that for clarity and consistency, that function should only be used for its return value, and shouldn't alter the object in place. I'll make a follow-up PR that changes that function, and also alters the usage here to match.

@wch wch merged commit cd3b84e into main Dec 7, 2023
@wch wch deleted the page-fillable-dependency branch December 7, 2023 01:30
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.

Spacing between elements within page_fillable() is off when no fill items are present
2 participants