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

Add parallel letter frequency #426

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kahgoh
Copy link
Member

@kahgoh kahgoh commented Mar 12, 2024

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Wonderful! One small note inline

config.json Outdated
Comment on lines 1835 to 1836
"practices": [],
"prerequisites": [],
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to have some practices and prerequisites here!

import gleam/dict.{type Dict}
import gleam/list
import gleam/option.{type Option, None, Some}
import gleam/otp/task
Copy link
Member

@lpil lpil Mar 17, 2024

Choose a reason for hiding this comment

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

This hasn't been taught yet by this point, and it's an indirect dependency so it'll emit a warning.

It would be best if the concept of an OTP task had been taught prior to having the student use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed the CI build failed as I had added the OTP task to the prerequisite, although it hasn't been added yet. I've since it removed it and raised #454.

config.json Outdated
"lists"
],
"prerequisites": [
"lists"
Copy link
Member

Choose a reason for hiding this comment

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

This is missing regex, options, strings, dicts, and OTP tasks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I think missed those earlier as I didn't seem them listed in the existing concepts. I've just added them and pushed.

With the dict one, I put maps in its place as I noticed the existing ones called it maps instead of dict and I assumed it would be updated as part #396. Let me know if you'd prefer me to change it to dict instead.

Copy link
Member Author

@kahgoh kahgoh Mar 18, 2024

Choose a reason for hiding this comment

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

Update - Nevermind, I just saw pull request #431 and decided to go ahead and update this pull request to use dicts to match.

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