Skip to content

Conversation

@gleachkr
Copy link
Contributor

@gleachkr gleachkr commented Sep 9, 2022

Fixed a tiny typo.

@CLAassistant
Copy link

CLAassistant commented Sep 9, 2022

CLA assistant check
All committers have signed the CLA.

@joaquinelio
Copy link
Member

joaquinelio commented Sep 9, 2022

👍
Not sure about "convention"
but it needs a "this something" or "this" alone for sure.

@gleachkr
Copy link
Contributor Author

gleachkr commented Sep 9, 2022

Yeah, "this convention" might be a little stuffy. What about a slightly bigger edit:

But following this pattern helps us replace Map with Set in certain cases with ease, and vice versa.

@joaquinelio
Copy link
Member

It's ok anyway, it doesn't need to be perfect.

mm...
if we overthink about it we can lose the original idea (it may sound dubious on purpose, like making excuses). so I prefer the first fix.

But now I am overthinking.

@joaquinelio
Copy link
Member

I'm not native English, still learning.
Without the fix, it sounds like a suggestion; am I right?

@gleachkr
Copy link
Contributor Author

gleachkr commented Sep 9, 2022

Hm... it doesn't quite sound like a suggestion to me. It's just saying that the callback having three arguments is a design choice that could have been made differently, but it has good reasons behind it.

But if putting it this way makes it hard to understand, then again, maybe that's not the best formulation. Just dropping the original edit to

That's for compatibility with Map where the callback passed forEach has three arguments. Looks a bit strange, for sure. But this may help to replace Map with Set in certain cases with ease, and vice versa.

would be OK too.

There's also maybe another tiny grammar nitpick for this paragraph. I think "the callback passed forEach" should be "the callback passed to forEach". I can do a separate PR or fold it into this one, if you agree with the edit.

@joaquinelio
Copy link
Member

joaquinelio commented Sep 10, 2022

I like the added "this" alone,
But this may help to replace
but Im nobody here, it's Ilya's kid.

sorry for
"Without the fix, it sounds like a suggestion; am I right?"
nothing to do with the repo,
tldr
it was my interpretation of the original, not what the author intended to say.
But may help to replace (...)
sounded to me like
But replacing (...) may help
I wasnt sure if was bad grammar or just a wrong way to put it.

@gleachkr
Copy link
Contributor Author

But may help to replace (...)
sounded to me like
But replacing (...) may help
I wasnt sure if was bad grammar or just a wrong way to put it.

Oh, I see! Yeah, to me, without the fix, the original just feels like a typo. It's definitely ungrammatical as is.

Simplify wording
@gleachkr
Copy link
Contributor Author

I switched to the plain "this". If something else would be better, or if this should be squashed to a single commit, just let me know.

@joaquinelio
Copy link
Member

I'm not the one to say
sorry, maybe the first fix was ok

"passed to" is needed too I think

And thanks 👍 for the explanation.

@iliakan iliakan merged commit 202e625 into javascript-tutorial:master Sep 20, 2022
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.

4 participants