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

Update Fruit Picker -- remove conceptual leaps #1725

Merged
merged 16 commits into from
Jun 29, 2022
Merged

Conversation

neenjaw
Copy link
Contributor

@neenjaw neenjaw commented Apr 11, 2022

This exercise was lacking in its ability to teach the concept of callback functions --- a programming technique to facilitate control flow and code re-use.

This is an attempt to simplify this concept. As such it removes the taught concept of arrow-functions, so there is now no exercise which teach this concept. Likely arrow functions should be taught in closer with the concept of this as its main benefit (aside from a more concise syntax) is the difference in its handling of this.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 11, 2022

Dear neenjaw

Thank you for contributing to the JavaScript track on Exercism! 💙
You will see some automated feedback below 🤖. It would be great if you can make sure your PR covers those points. This will save your reviewer some time and your change can be merged quicker.

  • 📜 The following files usually contain very similar content.

    • concepts/<concept>/about.md
    • concepts/<concept>/introduction.md
    • exercises/concept/<exercise>/.docs/introduction.md

    Please check whether the changes you made to one of these also need to be applied to the others.

  • 🧦 If you changed the function signature or the JSDoc comment in the exemplar file (.meta/exemplar.js) or the stub file (<exercise>.js), make sure the change is applied to both files.

  • ✍️ If your PR is not related to an existing issue (and is not self-explaining like a typo fix), please make sure the description explains why the change you made is necessary.

  • 🔤 If your PR fixes an easy to identify typo, if would be great if you could check for that typo in the whole repo. For example, if you found Unicdoe, use "replace all" in your editor (or command line magic) to fix it consistently.

Dear Reviewer/Maintainer

  • 📏 Make sure you set the appropriate x:size label for the PR. (This also works after merging, in case you forgot about it.)

  • 🔍 Don't be too nit-picky. If the PR is a clear improvement compared to the status quo, it should be approved as clear signal this is good to be merged even if the minor comments you might have are not addressed by the contributor. Further improvement ideas can be captured in issues (if important enough) and implemented via additional PRs.

  • 🤔 After reviewing the diff in the "Files changed" section, take a moment to think about whether there are changes missing from the diff. Does something need to be adjusted in other places so the code or content stays consistent?

Automated comment created by PR Commenter 🤖.

@neenjaw
Copy link
Contributor Author

neenjaw commented Apr 11, 2022

First attempt here just to simplify this exercise and improve the progression to make it more clear that we are trying to teach that call backs are just functions given to some function to invoke (or not) to induce some behaviour.

Feedback welcome.

Todo:

  • second part of exercise design
  • test file
  • update introduction
  • update instructions

@junedev
Copy link
Member

junedev commented Apr 12, 2022

@neenjaw Great that you want to work on this topic. Is your PR related to #1634? There was no issue link set so if you think your work will cover what is mentioned in that issue, I would like to assign you there. Then I know, I don't have to pick up that issue.

@SleeplessByte SleeplessByte self-requested a review April 13, 2022 11:21
- use fake declared modules
- all synchronous callbacks which do not expect return values
@neenjaw
Copy link
Contributor Author

neenjaw commented Apr 16, 2022

@junedev, no wasn't initiated with a specific issue in mind. Received a lengthy discussion of someone's frustration and decided to revisit this exercise to remove/clarify/straighten a few concepts.

Plan is to focus in on the idea that callbacks are just functions passed to other functions to execute on their terms.

@neenjaw
Copy link
Contributor Author

neenjaw commented Apr 16, 2022

However, I think it would be reasonable to link (or assign) that issue to make sure those ideas are covered.

- introduction tidied
- about tidied
- created empty helper module function to satisfy eslint checks
- removed arrow-function concept from taught concepts
@neenjaw neenjaw marked this pull request as ready for review May 2, 2022 03:18
@neenjaw
Copy link
Contributor Author

neenjaw commented May 2, 2022

@SleeplessByte @junedev, this is now ready for your review

@junedev
Copy link
Member

junedev commented Jun 9, 2022

@neenjaw Sorry for the long delay in reviewing this. @SleeplessByte and me have been very busy. Things are normalizing on my end now so I will probably get around to reviewing this in the next 1-2 weeks.

Copy link
Member

@junedev junedev left a comment

Choose a reason for hiding this comment

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

I finally finished the review. If you don't have time to address the comments, leave me a note and I can finish up for you if you want.

concepts/callbacks/about.md Outdated Show resolved Hide resolved
concepts/callbacks/about.md Outdated Show resolved Hide resolved

If the `callback` function _returns_ a boolean or boolean-like value, which is intended to be used (as opposed to a throwaway return value), it's called a predicate.
## Specific examples of callback functions
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about having a the category for all the forEach, map, filter, etc methods in JavaScript? I often notice beginners don't understand that the argument is a callback function that will be applied to the elements. I think it would be good to include it as a first example category in the list. However, I am not sure how to name the category ... maybe "Iteration Callbacks" ... idk ... maybe you have a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't those array methods taught in other concept exercises? Shouldn't it be those exercises' responsibility to build on prior knowledge and point out that those array functions are callbacks in disguise?

Copy link
Member

Choose a reason for hiding this comment

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

We (DJ and me) have the guideline that about.md files are not bound by the syllabus progression because they are used for later reference. So if something is a good fit to explain a concept, we would include it in the about.md no matter where it would show up in the concept tree (if at all). My feeling is that the current example categories have some "overweight" on the complex side. That is why I though a simpler category would balance it out. Not a blocker though. Feel free to click resolve if you prefer it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detail added in dd155a5, sub-titled as "Callbacks in disguise"

config.json Outdated Show resolved Hide resolved
exercises/concept/fruit-picker/.meta/exemplar.js Outdated Show resolved Hide resolved
exercises/concept/fruit-picker/fruit-picker.spec.js Outdated Show resolved Hide resolved
@junedev junedev removed the request for review from SleeplessByte June 11, 2022 17:33
@junedev junedev added the x:rep/large Large amount of reputation label Jun 11, 2022
to be addressed later as they are not explicitly taught in the context of this exercise.
- added basic example to about
- added detail about array prototype function which use callbacks
@neenjaw
Copy link
Contributor Author

neenjaw commented Jun 25, 2022

@junedev see commits with revisions based on the reviewed feedback 😄

config.json Outdated Show resolved Hide resolved
@junedev junedev merged commit cede504 into exercism:main Jun 29, 2022
@junedev
Copy link
Member

junedev commented Jun 29, 2022

@neenjaw Thanks a lot for the update!

@SleeplessByte
Copy link
Member

Love the changes <3

@dunndeft
Copy link

dunndeft commented Jul 13, 2022

Hello. How can I assist in updating the hints? This is my first attempt at contributing to Exercism.

@junedev
Copy link
Member

junedev commented Jul 13, 2022

@dunndeft Sure. 🙂 Please leave a comment in #1833 so I can assign the issue to you. Make sure to read the documentation linked in the issue so you know the intent and format for the hints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:rep/large Large amount of reputation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[exercises/concept/fruit-picker] Exercise requires users to be familiar with the throw statement
4 participants