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

The Farm: Improve introduction #2011

Closed
1 of 2 tasks
junedev opened this issue Dec 19, 2021 · 9 comments
Closed
1 of 2 tasks

The Farm: Improve introduction #2011

junedev opened this issue Dec 19, 2021 · 9 comments

Comments

@junedev
Copy link
Member

junedev commented Dec 19, 2021

Follow up from #2009

The tests for "The Farm" currently don't cover what should happen when a negative fodder amount and a non-scale error is returned. Go's general guideline is to not use the result if there was an error so passing on the error takes precedence over handling the negative amount.

Tasks

  • Add a test case for a negative fodder amount and a non-scale error. The expected outcome of DevideFood is 0 and the error that was returned from FodderAmount.
  • Make sure the errors concept and the exercise introduction mention the guideline for usually not using the result in case there was error.
@junedev junedev added help wanted x:action/improve Improve existing functionality/content x:knowledge/none No existing Exercism knowledge required x:module/concept-exercise Work on Concept Exercises x:type/content Work on content (e.g. exercises, concepts) x:size/small Small amount of work labels Dec 19, 2021
@junedev junedev changed the title The Farm: Add test case The Farm: Improve introduction Dec 27, 2021
@junedev
Copy link
Member Author

junedev commented Dec 27, 2021

A quote from @andrerfcsantos on Slack regarding the topic of the open task here:

In Go, when you return a non-nil error in a function that also returns other things, the caller of the function is supposed to ignore all the other things except the error. This is unless the documentation of the function says otherwise, but I never really saw a function return an error but make you pay attention to the other values.
This means that n theory, when you return an error, you can return anything in the other values - those other values aren't supposed to be looked at anyway because you are returning an non-nil error. But returning the zero value makes the code easier to read, as you are signaling there's nothing special going on there.

@Lewiscowles1986
Copy link
Contributor

Lewiscowles1986 commented Jan 27, 2022

To be quite honest, I think this might be a problem with the errors introduction, that it does not support or show learners the "Handle specific errors, but return others" explicitly.

    if err != nil && err != ErrScaleMalfunction {
    	return 0.0, err
    }

I Was most frustrated as I did not see language I recognised as saying "Hey, we want to handle this error, but early exit all others."

It pointed out a problem with how I approach golang; which I'll try to work on. But it could catch up anyone who is not an experienced and deep; regular gopher.

I'd in-fact read the exercise as "We will only expect this error".

How to communicate this... (suggestion)

Encapsulating error types and values

While it is advised above, to early return most errors that occur. There are occasions where defining behavior upon a specific type, or exported value of error is more appropriate. When that happens, only specific error types would not be immediately returned. This Encapsulation of errors is not unique to golang, but can be thought of as catching an Exception in other languages. The purpose might be to differentiate between things in-scope of a library or framework; and things that have not (perhaps yet) been considered.

The only other way I can think of is a flow diagram that basically hands the answer to all learners.

@junedev
Copy link
Member Author

junedev commented Jan 27, 2022

@Lewiscowles1986 You raise a good point there. However, I think what you are bringing up is unrelated to the open task in this issue. It would be great, if you could open a new issue instead and copy your comment in there. Then we can discuss a bit more in that new issue.

@junedev
Copy link
Member Author

junedev commented Jan 30, 2022

This issue and #2080 lead me to the question whether it is such a good idea to have the unusual pattern of "continue working with the result even though there was an error" as part of the concept exercise. As this is very rare, it might not be a good fit for the concept exercise that first introduces the student to the errors concept. It might even set a bad example. @andrerfcsantos How do you feel about this? Do you think we should change the exercise? That might avoid some bending the introduction around it.

@andrerfcsantos
Copy link
Member

@junedev I agree we should change the exercise to not use the normal result of the function even though there was a non-nil error. Not only is a bit unrealistic like you said, but it might also unnecessarily make the exercise more complex.

I think the idea is to show how to check for specific errors, but that's probably best done in the Error Wrapping concept.

@Lewiscowles1986
Copy link
Contributor

So, While I think this should leave this exercise, I also think it could be a separate exercise to wrapping, which to my mind serves a different purpose and is not encapsulation.

Encapsulating an error and providing domain-specific knowledge / behavior is it's own standalone take on errors to wrapping. It is not a general concept, that people have a nephew who can't be fixed / changed cheaper than code. This is almost directly the opposite thing to wrapping the error, where for some reason that error detail would leak out to the world. You are not embedding past errors when encapsulating; you are exchanging an error for either another error, or providing a behavior for an error, that does not return an error.

The original github.com/pkg/errors basically attempts to implement manual stack unwrapping ALA exceptions by-proxy. Like an Exception, it captures a stack trace for all errors; and provides a Cause, which seems similar to sub-type sniffing, common among exceptions. I Know it comes from Dave Cheney, but I Really dislike the pattern and am glad that the golang core implementation strips back this. It's smearing leaky details all over the place, rather than encapsulate them. It's not encapsulation if you pass every last error in a chain down to a consumer that then has to deal with a lot more details.

@mantrobuslawal
Copy link

I'm happy to help on this task

@junedev
Copy link
Member Author

junedev commented Sep 5, 2022

@mantrobuslawal If you want to make some small change related to the original open issue at the top, feel free to go ahead and create a PR. However, I would advise against bigger rework as we are planning to structurally change this exercise in the future (move interfaces into its own exercise and instead teach errors and error wrapping in one exercise).

@junedev junedev removed help wanted x:action/improve Improve existing functionality/content x:knowledge/none No existing Exercism knowledge required x:module/concept-exercise Work on Concept Exercises x:type/content Work on content (e.g. exercises, concepts) x:size/small Small amount of work labels Sep 17, 2022
@junedev
Copy link
Member Author

junedev commented Sep 17, 2022

Here the issue for the rework I mentioned above: #2492
I am closing this issue in favor of the more general one. As said above, PRs are ok but I don't want to encourage too much "patching it up" work on this exercise.

@junedev junedev closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 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 a pull request may close this issue.

4 participants