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

[#25] - Added answers for conclusion #73

Closed
wants to merge 2 commits into from

Conversation

ruth-lim
Copy link

Fixes #25


<box type="info" seamless>

Remember to update other usages of `DeleteCommand` class to handle the change in type of Index.

Choose a reason for hiding this comment

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

Hi, really comprehensive answers here!
Just a nit: 'to handle the change in type of argument' might be better, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, done!

* No exception thrown.
* Reason: The command is correctly formatted and will edit the tags of the person at index 1 to "one", "two" and "three".

<panel header="Why didn’t the second tag with value “one” get added?">

Choose a reason for hiding this comment

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

I think instead of a panel inside a panel, it would be better if the additional information was presented in an info box instead.

And the question could be made clearer: Why aren't there two tags with value "one" after this command is executed?

Lastly, would this be more accurate?
... ParserUtil#parseTags() method when tags are added to a HashSet...

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Agreed that an info box would be clearer for the reader. Made the changes

damithc pushed a commit that referenced this pull request Sep 8, 2024
@damithc
Copy link
Contributor

damithc commented Sep 8, 2024

@ruth-lim Thanks for this PR. I've incorporated your changes via 2daab40

@damithc damithc closed this Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AB3: Tracing Code] Enhance conclusion to provide answers for students
3 participants