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

Implementation of String.prototype.padStart/padEnd. #174

Merged
merged 1 commit into from
Jan 22, 2016

Conversation

akroshg
Copy link
Contributor

@akroshg akroshg commented Jan 22, 2016

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.

@msftclas
Copy link

Hi @akroshg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Akrosh Gandhi). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@akroshg
Copy link
Contributor Author

akroshg commented Jan 22, 2016

This was reviewed and signed off earlier - but did not get pushed to master. Rebased commits and created new PR.
@abchatra could you review this? Thanks,

@abchatra
Copy link
Contributor

👍


Assert(fillerString->GetLength() > 0);

charcount_t fillLength = (charcount_t)(maxLength - currentLength); // Possible truncation.
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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.
@chakrabot chakrabot merged commit 6b3508b into chakra-core:master Jan 22, 2016
@ljharb
Copy link
Collaborator

ljharb commented Jan 22, 2016

(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

@abchatra
Copy link
Contributor

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.

@ljharb
Copy link
Collaborator

ljharb commented Jan 25, 2016

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 :-)

@abchatra
Copy link
Contributor

You can follow the change log for the technical preview here https://dev.windows.com/en-us/microsoft-edge/platform/changelog/

Example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants