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.

Sign up for GitHub

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

Jump to bottom

new concept: the-farm - issue 1374 #1423

Merged
merged 10 commits into from
Mar 16, 2021
Merged

new concept: the-farm - issue 1374 #1423

merged 10 commits into from
Mar 16, 2021

Conversation

brugnara
Copy link
Contributor

@brugnara brugnara commented Feb 19, 2021 •
edited
Loading

Hi everyone.

Here's a concept I thought.

Should cover error-handling concept (#1374)

Feedbacks are as always very welcome.

PS: I did because I thought it was ok, if it's not the case, no problem at all.

Thank you for considering leaving your thoughts 🙏

tehsphinx
tehsphinx reviewed Feb 20, 2021
View reviewed changes
exercises/concept/the-farm/.meta/design.md Outdated Show resolved Hide resolved
exercises/concept/the-farm/.docs/instructions.md Show resolved Hide resolved
exercises/concept/the-farm/.docs/instructions.md Outdated
Comment on lines 21 to 23
You will be passed a weightFodder function which returns the amount of fodder
available but also an error, sometimes. You should not handle that error,
return it to the `FEED-M-ALL`, instead.
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 not really clear to me. Maybe mention the function signature to be implemented like below?

Copy link
Member

Choose a reason for hiding this comment

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

Ok... looking at the example implementation I realized only one function is to be implemented. Maybe make this clear from the start.

Copy link
Member

Choose a reason for hiding this comment

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

See my suggestion below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the use of errors.Is.

exercises/concept/the-farm/.docs/instructions.md Outdated Show resolved Hide resolved
exercises/concept/the-farm/.docs/introduction.md
@@ -0,0 +1,70 @@
# Introduction
Copy link
Member

@tehsphinx tehsphinx Feb 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Add wrapping of errors and checking of error types in the intro, if that is something you decide this exercise should teach as well.

tehsphinx
tehsphinx approved these changes Feb 22, 2021
View reviewed changes
Copy link
Member

@tehsphinx tehsphinx 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 made one more suggestion. But this looks good 😄

ekingery
ekingery approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@ekingery ekingery left a comment

Choose a reason for hiding this comment

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

Looks good overall, this is a fun exercise you have devised, @brugnara! Given I have not created a concept exercise myself, I'll defer to others as to any further revisions and how this will fit into the V3 launch.

concepts/errors-handling/about.md Outdated Show resolved Hide resolved
concepts/errors-handling/about.md Outdated Show resolved Hide resolved
concepts/errors-handling/about.md Outdated Show resolved Hide resolved
@ekingery
Copy link
Contributor

ekingery commented Mar 1, 2021

I spoke a bit too soon on the approval. I forgot, someone pointed out that the errors concept already exists. So, rather than creating errors-handling from scratch, @brugnara you'll want to merge your content with the existing concept here: exercises/concept/errors.

@brugnara
Copy link
Contributor Author

brugnara commented Mar 2, 2021

I spoke a bit too soon on the approval. I forgot, someone pointed out that the errors concept already exists. So, rather than creating errors-handling from scratch, @brugnara you'll want to merge your content with the existing concept here: exercises/concept/errors.

@ekingery #1389 this list means to me these are different concepts. Am I wrong?

@tehsphinx
Copy link
Member

There are supposed to be 2 concepts: errors and error-handling. I wouldn't merge them.

@ekingery
Copy link
Contributor

ekingery commented Mar 2, 2021 via email

@tehsphinx
Copy link
Member

@brugnara If you are done from your side, turn it into a non-draft PR.

@brugnara brugnara marked this pull request as ready for review March 6, 2021 18:32
@ekingery ekingery merged commit 4f94409 into exercism:main Mar 16, 2021
@ekingery
Copy link
Contributor

Apologies for the delay in merging! I forgot this was waiting on me to actually do the merging. Nice work @brugnara, and thanks to everyone for the code reviews and suggestions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haguro haguro haguro left review comments

@tehsphinx tehsphinx tehsphinx approved these changes

@ekingery ekingery ekingery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brugnara @ekingery @tehsphinx @haguro