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

Test end to end #30

Closed
wants to merge 11 commits into from
Closed

Test end to end #30

wants to merge 11 commits into from

Conversation

jimbarrett27
Copy link
Owner

No description provided.

@jimbarrett27 jimbarrett27 requested a review from naslundx August 20, 2024 15:52
@jimbarrett27 jimbarrett27 marked this pull request as draft August 20, 2024 16:06
st.lists(st.text(ascii_letters + digits + punctuation, min_size=1), min_size=1)
)

frequencies = draw(
Copy link
Collaborator

Choose a reason for hiding this comment

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

ℹ️ This is nice - just a thought from reading this: It might be useful to also support supplying an already-created Counter object, especially if people have already processed huge lists of words.

Copy link
Owner Author

Choose a reason for hiding this comment

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

good idea, I'll make an issue

):
if seed is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

ℹ️ Suggestion: We should not set the global seed, but we can keep our own local Random instance. That way, people can get determinism from wrdcld without upsetting the randomness in the rest of their script (whatever it may be).

See: https://stackoverflow.com/a/37356024

Copy link
Owner Author

Choose a reason for hiding this comment

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

but we don't touch the random state unless a seed is supplied, no? If the user is controlling it globally then they just wouldn't touch this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The point is we do touch the (global) random state. If the user wanted to do that, they could just call random.seed themselves before calling make_word_cloud. If we instantiate our own random generator and only seed that one, then this argument only affects randomness within wrdcld, and nothing else in a user's library or script. They may want "true" randomness elsewhere but a handy way to get determinism from wrdcld.

(But we can do this in a separate PR for sure)

@@ -86,7 +86,7 @@ def fill_next_word(word, available_rectangles, image, font, frequency):
options.append("vertical")

if not options:
print(f"skipping word '{word}', couldn't find a good rectangle")
# print(f"skipping word '{word}', couldn't find a good rectangle")
Copy link
Collaborator

Choose a reason for hiding this comment

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

ℹ️ Idea (not for this PR perhaps): We move this to a logger, and add a silent: bool flag to the main function that allows people to toggle this output on or off.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agreed! I would also like to give some more thought to how silently we fail, when there is a word which is high up in the frequency list, but that we can't fit in the diagram for whatever reason (assuming this happens at all)

@jimbarrett27 jimbarrett27 marked this pull request as ready for review August 21, 2024 14:19
@jimbarrett27
Copy link
Owner Author

the tests and stylechecks didn't run for some reason, I'll investigate later...

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.

2 participants