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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Underscore.string #11963

Merged
merged 4 commits into from Apr 4, 2016
Merged

Upgrade Underscore.string #11963

merged 4 commits into from Apr 4, 2016

Conversation

andy-armstrong
Copy link
Contributor

FEDX-117

This is a second attempt to upgrade Underscore.string after this PR had to be reverted (https://github.com/edx/edx-platform/pull/11631). The hope is that the change in approach to deploying npm libraries (https://github.com/edx/edx-platform/pull/11938) will mean that there won't be strange failures in the bundled version.

Sandbox

Testing

  • i18n
  • RTL
  • Data is properly encoded in HTML templates to avoid XSS
  • Unit, integration, acceptance tests as appropriate
  • Analytics
  • Performance
  • Database migrations are backwards-compatible

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 馃憤.

Post-review

  • Squash commits

@andy-armstrong andy-armstrong force-pushed the andya/upgrade-underscore-string branch from 338ba51 to a132625 Compare March 25, 2016 21:21
@andy-armstrong
Copy link
Contributor Author

@bjacobel You'll need this PR's version of the UI Toolkit for some of my changes here to work:

openedx/edx-ui-toolkit#46

@andy-armstrong
Copy link
Contributor Author

@bjacobel Another way to remove usages of Underscore.string is to replace calls to _.sprintf with StringUtils.interpolate. There are 53 such usages so that may be too much for one pull request, but most of them are in Underscore templates which are very self contained and easily tested.

@andy-armstrong
Copy link
Contributor Author

There are also three usages of _.trim which are entirely in test files. These can just be changed to use the native string's trim method, since it works in all our supported browsers.

@bjacobel bjacobel force-pushed the andya/upgrade-underscore-string branch from dc20c08 to 74ec5ba Compare March 29, 2016 17:20
@bjacobel
Copy link
Contributor

Sandbox coming online at underscorestring.sandbox.edx.org.

@bjacobel bjacobel force-pushed the andya/upgrade-underscore-string branch from 8fe6655 to 41adf01 Compare March 31, 2016 19:27
@bjacobel
Copy link
Contributor

bjacobel commented Apr 1, 2016

jenkins run bokchoy

1 similar comment
@bjacobel
Copy link
Contributor

bjacobel commented Apr 1, 2016

jenkins run bokchoy

@bjacobel
Copy link
Contributor

bjacobel commented Apr 1, 2016

@andy-armstrong Tests now passing, would be worth doing a run through of manual tests on sandbox as well though I think. I edited your initial comment on this thread to include the pull request template.

cc @AlasdairSwan

@andy-armstrong
Copy link
Contributor Author

馃憤 Looks great, @bjacobel.

@bjacobel bjacobel force-pushed the andya/upgrade-underscore-string branch from 6e0415c to 6df9f3d Compare April 4, 2016 14:18
@bjacobel bjacobel merged commit 9baae2f into master Apr 4, 2016
@bjacobel bjacobel deleted the andya/upgrade-underscore-string branch April 4, 2016 15:35
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.

None yet

2 participants