Skip to content

Conversation

wch
Copy link
Collaborator

@wch wch commented Dec 7, 2023

This is a follow-up to #861 (comment). Also closes #577.

This PR makes it so that as_fillable_container() and as_fill_item() do not modify their inputs.

Fixes #865

@wch wch requested review from cpsievert and schloerke December 7, 2023 01:50
Comment on lines +16 to +20
@contextlib.contextmanager
def private_seed_n(n: int = 0) -> Generator[None, None, None]:
with private_seed():
random.seed(0)
return func(*args, **kwargs)
random.seed(n)
yield
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The purpose of this new context manager is to make it so the code inside the with private_seed_n(): statement is clearer -- it will have the same form as it would if were called without the with private_seed_n():.

@wch
Copy link
Collaborator Author

wch commented Dec 7, 2023

@schloerke On a related note, I was looking at the functions tag_prepend_class(), tag_remove_class() tag_add_style() because they also mutate their input objects. Then I realized that those functions now exist as methods on the Tag class in htmltools. The methods mutate the object and return the object, but that feels more OK than outside functions that mutate the input and return it.

Is there any reason not to switch to those methods from htmltools? That would remove the need for those functions, and also make the code feel less weird.

@gadenbuie
Copy link
Collaborator

gadenbuie commented Dec 7, 2023

@wch I was looking into switching to the .add_class() method for appending the fillability classes. I was coming at this from the perspective of not putting utility classes first in the markup. I started working on making that change earlier today, but if it makes sense to roll that into this PR, I think that'd be a good change.

Edit: I opened an issue to track/explain this comment: #865

@wch
Copy link
Collaborator Author

wch commented Dec 7, 2023

I've removed the tag_ functions in favor of the Tag methods, and also made the fill classes come at the end. This closes #865.

@schloerke
Copy link
Collaborator

schloerke commented Dec 7, 2023

  • changelog entry

@wch wch merged commit ceb6311 into main Dec 7, 2023
@wch wch deleted the fill-no-mutate branch December 7, 2023 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants