Skip to content
This repository was archived by the owner on Mar 14, 2020. It is now read-only.

[Serialize] Add 'find', 'add', 'remove' sandbox header functions#206

Closed
DonnaWuDongxia wants to merge 1 commit into
intel:masterfrom
DonnaWuDongxia:sandbox-header
Closed

[Serialize] Add 'find', 'add', 'remove' sandbox header functions#206
DonnaWuDongxia wants to merge 1 commit into
intel:masterfrom
DonnaWuDongxia:sandbox-header

Conversation

@DonnaWuDongxia

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/js/serialize.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why return when header is not found? I think we should do the reverse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the findResult is null, it means we don't have such type of head.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think addSandboxHeader should add a header when it can't find that header. If you return when not finding the header, how can you add a new header?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I think I misunderstood the return value of findSandboxHeader. When "null" is returned, it means something wrong happened. Otherwise an object will be returned even if there's no header found. A little tricky...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can have a more straightforward interface for findSandboxHeader. From end user's point of view, I thin hasSandboxHeader is enough, which returns only true or false.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we return false when some error happens?

@DonnaWuDongxia

Copy link
Copy Markdown
Contributor Author

replaced by #207

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants