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

Can I have it with milk? #18

Open
wants to merge 4 commits into
base: mistress
Choose a base branch
from

Conversation

blazgocompany
Copy link
Contributor

Yep, it's me again. I hope I'm not pestering you.

Copy link
Owner

@towerofnix towerofnix left a comment

Choose a reason for hiding this comment

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

Hmm, this might be a contentious subject... of course you can have it with milk, and indeed any milk would probably be quite delicious. But is cinnamon milk truly the best? Is not the beauty of Cinnamon Toast Crunch in milk, that it flavors the milk, that its sugar-cinnamon goodness is absorbed into the milk? In this regard cinnamon milk is not only superfluous, but actually obscures that oh-so-essential cereal-milk action!

I think if we want to be a serious reference on why kids love the taste of Cinnamon Toast Crunch (and we surely do), then we might take care to acknowledge Cinnamon Toast Crunch in all its delightful dynamics.

We should also try to match the larger JavaScript ecosystem. getMilkCombinations... is a plural name, so one would expect it to return an array or otherwise iterable (such as a generator). Apart from the cinnamon-philosophical quandary above, either the function should be renamed (the export is OK), or the contents should return an iterable of milk combinations that go with cinnamon toast crunch.

Thank you for your contributions, of course!

@blazgocompany
Copy link
Contributor Author

Great point... Ill think about it

@blazgocompany
Copy link
Contributor Author

Your language in the first paragraph is truly amazing! I don't know if this is what you're looking for (see the commit) but I hope that's good!

index.js Outdated
"Soy Milk",
"Cinnamon Milk"
];
}
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, this is almost there!! I'm a stickler for literals, so can we try a syntax that's more like...

module.exports.whichMilkCanIHaveItWith =
  function* getMilkCombinations() {
    yield "with 2% milk"
    yield "with whole milk"
    yield "with almond milk"
    yield "with coconut milk"
    yield "with soy milk"
    yield "with cinnamon milk"
    yield "with your favorite milk"
    yield "or even dry"
  }

Maybe we should add an example in the examples folder too, something like this:

const getReasonWhyKidsLoveTheTasteOfCinnamonToastCrunch = require(
  'why-do-kids-love-the-taste-of-cinnamon-toast-crunch')

const getMilksICanHaveCinnamonToastCrunchWith =
      getReasonWhyKidsLoveTheTasteOfCinnamonToastCrunch.canIHaveItWithMilk

for (const milkICouldHaveItWith of getMilksICanHaveCinnamonToastCrunchWith()) {
    console.log(`I could have it ${milkICouldHaveItWith}`)
}

I don't know what to name this example though LOL. Just make sure you use an underscore (_) between each word, like the other files in the examples folder.

Also, please put a line break above the module.exports line - you can look at the picture below to see how the code you added doesn't quite fit the same style as the other ones, but that's just a simple fix.

Screenshot of bottom four functions in this file

@blazgocompany
Copy link
Contributor Author

Ok sorry I didn't see this. Ill work on it.

"Cinnamon Milk"
];
}
module.exports.whichMilkCanIHaveItWith =
Copy link
Owner

Choose a reason for hiding this comment

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

Super close! Thank you! Just add a blank line above this line, please.

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.

None yet

2 participants