-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Bug - : 05_sumALL: Returning ERROR with negative numbers #255
Comments
Can you please please assign it to me? I've already passed all the tests. As soon as you assign it to me I'll make a pull request. |
@Houndoom I've made a PR on the following issue. Please review it. |
@Mahir1015 Thanks very much for looking into this! However, I am not a part of The Odin Project and hence am not able to assign issues or merge PRs, you may wish to approach someone on the discord who has the access to do so |
@Houndoom Anytimee! Can you please provide me the link of discord server? |
@Mahir1015 You can find it on this page: https://www.theodinproject.com/lessons/foundations-join-the-odin-community |
@Houndoom Thanks! |
@Houndoom would you be able to explain further the issue with the 4th test in this exercise? If a test fails, the intent is that a user refactors their function(s) until that test (and all previous tests) pass. In this case the test should pass by returning a string "ERROR" if any negative numbers are passed to the function. |
@thatblindgeye There is nothing in the question that suggests that the task should fail for negative numbers. The term "integers" includes negative integers as well, and hence I would expect this function to work with negative integers. If the intention was for it to fail with negative integers, suggest to amend the question to state "positive integers". |
@Houndoom Ah okay, I see what you mean. You're correct, we should either update the README to mention the function should only work for positive numbers, or add/update tests to work with negative numbers. Do you feel strongly one way or the other, coming from the point of view as a user? |
@thatblindgeye I have no strong preference, but perhaps a slight preference for updating the README as it would be a good introduction to edge cases, and more learning is always good. If this route is taken, suggest to have an appropriate hint to guide the user to think about the edge case, and the desired return value of "ERROR" would have to be explicitly stated. |
Actually as I type out a task list and think it over a little more, I'm thinking we may not really need to really mention this in the exercise description. While it isn't said at first that the function should only work for positive numbers, having a test that fails when a negative number is passed might have been intentional. It requires a user to have to refactor their function to continually pass the tests that may be added as edge cases are found. So a function that just works with any numbers passed in at first is good as long as the first test passes, then test 2 and test 3. Once the user gets to test 4, it's kind of like, "Oh, we've found out we only need this function to work for positive integers and to error when any negative integer is passed in. Let me refactor my function so this test will pass now..." Which can be how writing functions and tests work in a real world scenario. You may not have every edge case thought of from the start, and the original spec may not include all the information (some info may not be known at that time). Once you have something created and the person who asked for it sees it, they may realize, "Oh you know what, we actually want to prevent this thing..." |
That sounds reasonable. In a sense, we can take the tests as part of the specs. Personally, it feels off because rather than it being a case of not enough information, it seems like a contradiction to state integers when it actually means positive integers. In addition, in the context of a homework question, I would expect the description to state exactly what is required. There was also no indication that descriptions may be incomplete, and that tests may provide "additional information", and no explanation that this was meant to teach some of the considerations that might occur in a real world scenario, so it was just confusing to me. Of course, that is my personal opinion. It does not change the fact that the aim is to write a code that passes all the tests, and the current formulation presents no practical impediment to this goal. If the Odin team does not feel that there is a problem, I will accept this decision. |
I definitely get your point of view. Maybe it'd be beneficial if we mentioned somewhere in the README that certain tests may require the user to refactor their code or that the description in each exercise's README may not provide the full story. I personally might be more of the opinion that it kinda gets into semantics as to whether there's significant benefit in mentioning that vs a user discovering it on their own, but I'm but one man 😆 Let me ping @TheOdinProject/javascript to see if anyone else has opinions on this, and if any other users see this issue and have opinions definitely let us know |
Had a look through and I see sensible points on both sides.
At first, I thought people would just jump to doing everything in a conditional block if both params are positive integers, or perhaps even a guard clause for negative integers, and that this would therefore not run into the "refactor now you've bumped into a failing test" scenario. But looking at the tests, if they did this then they would still fail because the test specifically expects the string So they'd still need to refactor to return the expected string. How does that sound? |
I'd be good with just updating the README with that quick change. @Houndoom would you be interested in making this update? I know you didn't check the box for being interested in your original post, but in case you've changed your mind. |
Thanks @thatblindgeye for the offer, I'm not interested in performing the update |
Complete the following REQUIRED checkboxes:
Bug - location of bug: brief description of bug
format, e.g.Bug - Exercises: File type incorrect for all test files
The following checkbox is OPTIONAL:
1. Description of the Bug:
The 4th test for this task checks that the function returns ERROR with negative numbers. However, the question did not state that the integers had to be non-negative, and it is in fact possible to obtain a proper answer even with negative numbers.
2. How To Reproduce:
3. Expected Behavior:
4. Desktop/Device:
5. Additional Information:
The text was updated successfully, but these errors were encountered: