-
Notifications
You must be signed in to change notification settings - Fork 0
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
required changes are included #15
required changes are included #15
Conversation
I didn't got the idea of instructing about creation of |
This was referring to teaching a student that they can call the constructor ( We are teaching comprehensions in a separate exercise. 🤔 Wondering now as I re-read this if we really need a specific exercise task for creating a Worst case scenario is that we will need to do another change farther down the line to the exercise. But that is OK. 😄 These are all works in progress! Thoughts? For you as a learner, would practicing the difference between |
I think we may also want to revise the wording and function name on task number three, to make a distinction between decrementing inventory counts vs removing the item (and all counts) from the inventory (which is your new task #4). Suggest something like: 3. Decrement stock in the inventoryImplement the
Item counts in the inventory should not fall below 0. If the number of times an item appears on the list exceeds the count available, the quantity listed for that item should remain at 0 and additional requests for removing counts should be ignored.
Of course that also means that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mukeshgurpude -- Thank you for all the work on this. 🌟 Apologies for all the delays in reviewing it.
Overall, this looks pretty good! I did have some language suggestions, and an additional thought on task 3 re-wording.
This PR is also failing CI -- I think that's due to some upstream changes from today that you didn't get the chance to pull in. If you can, please do that. If you can't -- we can merge this into my original branch, and resolve the issue there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sigh. GitHub got me again. I meant to request changes, but it went ahead and "approved". Again, I think the changes are straightforward and should be quick.
Co-authored-by: BethanyG <[email protected]>
That's why I think, there is no need to talk about
I'll do that |
Agree. Let's leave that particular change off. 😄 I will go ahead and wait for that last task 3 change, and then we can merge this in! YAY! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 🌟 This looks good to go. Many thanks!
I'm looking forward to contribute more in Exercism!!🥲 |
Since, there's already a draft pull request from this branch, I thought about just pushing changes to this branch only.
Edit design.md doc to add dict.pop() to the design
Edit Introduction.md to add dict.pop() usage.
Edit instructions.md to add new tasks:
creation of dict literals(not needed. See comments)Add appropriate hint as needed in hints.md for dict.pop() usage
Edit exemplar.py to include new idiomatic solution for newly added tasks
Edit dicts_test.py file to add new tests for dict.pop() task.
Edit dicts.py stub file to add new tasks.
Edit test file to explicitly list imported functions/methods by name instead of using import *
Check/edit all markdown files to conform to the Exercism Markdown Specifications