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

Refactor scrabble module #276

Merged
merged 4 commits into from
Feb 21, 2024
Merged

Refactor scrabble module #276

merged 4 commits into from
Feb 21, 2024

Conversation

RichDom2185
Copy link
Member

@RichDom2185 RichDom2185 commented Feb 18, 2024

Description

Generates scrabble letters using .map on words to:

  • Improve editor responsiveness (smaller tokenization, less number of lines, etc.)
  • Allow for single source of truth

Also generates the tiny dataset using .filter every 100 words.

The correctness is guaranteed by the introduction of additional tests, ensuring they stay the (snapshot-checking).

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

This allows for single source of truth, and also improves editor
responsiveness (cutting down tokenization time, etc.).

The correctness is guaranteed by the introduction of snapshots in tests.
@RichDom2185 RichDom2185 self-assigned this Feb 18, 2024
Copy link
Member Author

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

I've generated the snapshots once before the refactor (refer to commit history) and can confirm that the refactor does not change the snapshots.

@RichDom2185
Copy link
Member Author

@martin-henz can I check what exactly qualifies for scrabble_words_tiny? Or is it handpicked? Perhaps we can generate it in a similar way, using array functions on scrabble_words.

@martin-henz
Copy link
Member

martin-henz commented Feb 20, 2024

@martin-henz can I check what exactly qualifies for scrabble_words_tiny? Or is it handpicked? Perhaps we can generate it in a similar way, using array functions on scrabble_words.

I remember playing with the scrabble module in preparation for the PA last semester. There was a concern that the full dataset would be too large for some student's computers, so I came up with scrabble_words_tiny. I think I sampled the full set at intervals of 100:

display(array_length(scrabble_words));          // 172820
display(array_length(scrabble_words_tiny));     // 1729

[We ended up not using the scrabble module for the PA.]

@RichDom2185
Copy link
Member Author

@martin-henz can I check what exactly qualifies for scrabble_words_tiny? Or is it handpicked? Perhaps we can generate it in a similar way, using array functions on scrabble_words.

I remember playing with the scrabble module in preparation for the PA last semester. There was a concern that the full dataset would be too large for some student's computers, so I came up with scrabble_words_tiny. I think I sampled the full set at intervals of 100:

display(array_length(scrabble_words));          // 172820
display(array_length(scrabble_words_tiny));     // 1729

[We ended up not using the scrabble module for the PA.]

Ok, I've used .filter((_, i) => i % 100 === 0) to generate scrabble_words_tiny and it seems to be an exact match with the snapshot.

This PR is ready now, thank you!

@RichDom2185 RichDom2185 enabled auto-merge (squash) February 21, 2024 01:35
Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

Nice!

@RichDom2185 RichDom2185 merged commit 014b481 into master Feb 21, 2024
3 checks passed
@RichDom2185 RichDom2185 deleted the refactor-scrabble branch February 21, 2024 01:42
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