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

Define string concatenation #187

Merged
merged 2 commits into from
Jan 4, 2018
Merged

Define string concatenation #187

merged 2 commits into from
Jan 4, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 4, 2018

Fixes #185.


Preview | Diff

infra.bs Outdated
</ol>

<p class=example id=example-string-concatenate>To serialize a set <var>set</var>, return the
<a for=string>concatenation</a> of <var>set</var> with U+0020 SPACE.
Copy link
Member

Choose a reason for hiding this comment

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

Here you are conflating a single code point and an entire string. Not sure whether we should be more careful, or just let it slide. (Maybe have a clause somewhere saying that single code points can be interpreted as strings when desired.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was wondering about that. I was hoping we could just cast between those when needed. Same with sets and lists and such.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to add text for this though.

Copy link
Member

Choose a reason for hiding this comment

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

Seeing the usage sites it seems like a good idea to add, perhaps in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #188. Addressed the remainder.

infra.bs Outdated
@@ -486,6 +486,24 @@ implementations of just <a>JavaScript strings</a> for performance and memory rea

<hr>

<p>To <dfn export for=string lt=concatenate|concatenation>concatenate</dfn> a <a for=/>list</a> of
Copy link
Member

Choose a reason for hiding this comment

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

I envisioned this being located in the same hr-delimited section as the split operations.

infra.bs Outdated
@@ -486,6 +486,24 @@ implementations of just <a>JavaScript strings</a> for performance and memory rea

<hr>

<p>To <dfn export for=string lt=concatenate|concatenation>concatenate</dfn> a <a for=/>list</a> of
<a for=/>strings</a> <var>list</var>, with an optional separator string <var>separator</var>, run
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if "using" would be better than "with". "The concatenation of X with Y" almost sounds like the result should be "XY"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call.

@annevk annevk mentioned this pull request Jan 4, 2018
@annevk annevk merged commit af5f4d0 into master Jan 4, 2018
@annevk annevk deleted the annevk/concat branch January 4, 2018 19:13
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.

2 participants