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

Add list clone, subsets, supersets, intersection and union #155

Merged
merged 7 commits into from
Oct 3, 2017

Conversation

tobie
Copy link
Collaborator

@tobie tobie commented Sep 27, 2017

Closes #154.


Preview | Diff

infra.bs Outdated
<p>To <dfn export for=list,stack,queue,set>clone</dfn> a <a>list</a> |list| is to create a new
<a>list</a> |clone| and, <a for=list>for each</a> |item| of |list|, <a for=list>append</a> |item|
to |clone|, so that |clone| <a for=list>contains</a> the same <a for=list>items</a>, in the same
order as |list|.
Copy link
Member

Choose a reason for hiding this comment

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

If you're making this explicit, you'd also have to explicitly clone the item. Or is the intent that they point to the same item?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's always the question isn't it? I assumed we'd add deep cloning if the behavior you describe was ever required.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's reasonable.

infra.bs Outdated
|superset| (and conversely, |superset| is a <dfn export for=set>superset</dfn> of |set|) if,
<a for=list>for each</a> |item| of |set|, |superset| <a for=set>contains</a> |item|. Note that this
implies that an <a>ordered set</a> is both a <a for=set>subset</a> and a <a for=set>superset</a> of
itself.
Copy link
Member

Choose a reason for hiding this comment

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

Note should be in a <span class=note> or its own paragraph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

@annevk annevk requested a review from domenic September 28, 2017 10:48
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Cloning is subtle so I think we should add a bit more as I indicated. But otherwise this is quite nice.

@@ -715,6 +715,11 @@ performing a set of steps on each <a for=list>item</a> in order, use phrasing of
"<a for=list>For each</a> |item| of <var ignore>list</var>", and then operate on |item| in the
subsequent prose.

<p>To <dfn export for=list,stack,queue,set>clone</dfn> a <a>list</a> |list| is to create a new
Copy link
Member

Choose a reason for hiding this comment

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

I think we should expand on this in two ways:

NOTE: this is a "shallow clone", as the items themselves are not cloned in any way

plus something about how cloning a specific type of list (stack/queue/ordered set) produces a list which is also one of those things. Maybe with an example?

@tobie
Copy link
Collaborator Author

tobie commented Sep 28, 2017

Do you want me to rebase those into 4 distinct commits (clone, subset & superset, intersection, union) or are you planning to mash them altogether into one (I don't care either way).

@domenic
Copy link
Member

domenic commented Sep 28, 2017

I'd go for mash. Just wanting @annevk to sign off too before doing so.

@annevk annevk merged commit a3a49bc into whatwg:master Oct 3, 2017
@tobie tobie deleted the sets branch October 3, 2017 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants