-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implementation of String.prototype.padStart/padEnd. #174
Conversation
Hi @akroshg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
This was reviewed and signed off earlier - but did not get pushed to master. Rebased commits and created new PR. |
👍 |
|
||
Assert(fillerString->GetLength() > 0); | ||
|
||
charcount_t fillLength = (charcount_t)(maxLength - currentLength); // Possible truncation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this behavior intended ?
For instance the following code
var s = "1";
print(s.padStart(4294967297));
would print 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cellule Our string does not go more than maxcharcount.
so in this case there could be one of the two possibilities.
a) Throw OOM or b) wrap around.
I chose to do wrap around earlier, but now that you pointed that out I looked at other API (eg. repeat) and it throws in such cases.
I going to match with that and will throw on overflow condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throwing is preferred, if a correct result won't be returned 👍
Proposal for this ES7 feature is found at https://github.com/tc39/proposal-string-pad-start-end. As the name suggests padStart will add padding to retrieve string up to max length by adding when needed on the front side. The padEnd will add at the rear side. This feature is sitting behind ES7Builtins flag. Added tests.
(fwiw I'd reviewed this on prior iterations of this, on different PRs) (ノ◕ヮ◕)ノ*:・゚✧ woot! When will this be available in a shipping Edge browser? :-D |
Changes that make it into our ChakraCore GitHub master branch have a short journey to Chakra.dll. Code flows daily from GitHub to the internal repository from which builds of Chakra.dll are produced and then it flows into Windows and Microsoft Edge. While code flows quickly on this first leg of the journey, code flow from our internal branch to a Windows flighting branch is subject to any number of delays. So it is difficult to predict when your change in our GitHub repo will make it into a particular Windows flight. In my experience, it takes anywhere between 6 to 10 weeks for the changes to make it to windows technical preview. |
Understood, thanks! Where's the best place to keep up with it? It would be amazing, for example, if Edge release announcements included a link to the SHA of ChakraCore that is included :-) |
You can follow the change log for the technical preview here https://dev.windows.com/en-us/microsoft-edge/platform/changelog/ |
Implementation of String.prototype.padStart/padEnd.
Proposal for this ES7 feature is found at https://github.com/tc39/proposal-string-pad-start-end.
As the name suggests padStart will add padding to retrieve string up to max length by adding when needed on the front side. The padEnd will add at the rear side.
This feature is sitting behind ES7Builtins flag.
Added tests.