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

Update displayOperands to handle optional operands #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Linda-Njau
Copy link
Contributor

The displayOperands function previously did not distinguish between optional and required operands. This change updates the function to display optional operands within square brackets and position them at the end of the operand list.

Copy link
Owner

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

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

I thought the operands were in-order in the JSON, but it turns out they aren't. They should be, which would make your implementation much easier.

I've opened an issue, ThinkOpenly/sail#26, to ensure the operands are in-order in the JSON.

Let's hold these changes until that is addressed. When that is addressed, I suspect your original approach would be sufficient, which I presume kept a single list of operands, "all", and just walked through the operands and appended to that list.

src/App.js Outdated
Comment on lines 36 to 40
if (operands[i].optional) {
optional.push("[," + operands[i].name + "]");
} else {
required.push(operands[i].name);
}
Copy link
Owner

Choose a reason for hiding this comment

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

We don't have documented "code style" guidelines, but I suggest following the general approach elsewhere in the code, and use 4 spaces for each level of indentation. Here, only 1 space is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see...
I'll hold the changes for displayOperands till then.

4 spaces, got it!

@ThinkOpenly
Copy link
Owner

Were you able to test this?

@Linda-Njau
Copy link
Contributor Author

Yes, I was able to test it.
This is a direct copy from the website: vd,vs2,vs1,[,vm]

The displayOperands function previously did not distinguish between optional and required operands.
This change updates the function to display optional operands within square brackets.

`vd,vs2,vs1,[,vm]`
@Linda-Njau
Copy link
Contributor Author

I've rebased this branch as well.

@ThinkOpenly
Copy link
Owner

Yes, I was able to test it. This is a direct copy from the website: vd,vs2,vs1,[,vm]

Would it be convenient to get rid of that extra comma? To produce vd,vs2,vs1[,vm] instead?


for (let i = 0; i < operands.length; i++) {
if (operands[i].optional) {
all += comma + "[," + operands[i].name + "]";
Copy link
Owner

Choose a reason for hiding this comment

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

I suspect fixing the "extra comma" would entail something like:

all += "[" + comma + operands[i].name + "]";

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 this pull request may close these issues.

2 participants