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
[fs.path.append] Fix examples to show correct results for Windows #2098
Conversation
Looks fine to me -- @hubert-reinterpretcast, any opinions? |
I forgot to explain that where we say:
This is true even if the separator is backslash not forward slash, because equality is defined in terms of element-wise equivalence, not equality of the |
The comment "On Windows, backslashes replace slashes in the above yields" should be removed because it's only true for one of "the above yields" not both, so let's just keep the POSIX examples and Windows examples separate. |
Looks okay to me, but I could not make definitive statements with regards to the behaviour for Windows. |
I think this example needs more work: these "yields string-literal" examples seem like a bad expositionary device. (As is, I can't judge whether this change is correct, because the example is meaningless.) |
I'm surprised nobody's mentioned the fact that this example for Maybe we should just rip it out entirely, as I've lost the will to fix it. |
e3dbfe2
to
1a21a65
Compare
I'm fine with just removing the example. |
@jwakely: Ping. |
@BillyONeal: Hi -- would you perhaps like to take over this issue? |
It looks like @jwakely already resolved this with this change, and the discussion about "well the entire example section isn't very good" is a different issue entirely. I think we should merge this, as it is an improvement to the spec, and then move discussions of "operator=/ examples use operator/" to a different, lower priority issue. |
It's not just that it uses I agree that fixing the results is an improvement, even if the overall presentation is still nonsense. |
I've rebased and pushed a revised commit, PTAL. |
388d396
to
caab798
Compare
It seems like that comment is also unrelated to this change. (Indeed, it also affects the non-Windows examples here) |
Indeed. I've tried to address all the problems in the latest commit anyway. |
@jwakely, thanks for the updates. I've added a review comment. |
Also correct the fact that the results were shown as string literals, not paths, and that the examples for path::operator/=(const path&) were actually using the non-member operator/(const path&, const path&) instead. Also line up the comments to the 32nd column.
@hubert-reinterpretcast, is this agreeable now, in your opinion? |
Looks fine to me. |
The examples for path::append show the wrong results for Windows, not matching the description of the function. This has been discussed with Billy O'Neal who confirmed the correct results.