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.

Sign up for GitHub

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

Jump to bottom

SE-2176 Update doc comments for accuracy #109

Merged
merged 1 commit into from
May 8, 2020
Merged

SE-2176 Update doc comments for accuracy #109

merged 1 commit into from
May 8, 2020

Conversation

samuelallan72
Copy link

@samuelallan72 samuelallan72 commented Apr 30, 2020 •
edited by bradenmacdonald

Currently there are discrepancies between the official docs, the code, and the doc comments here. These were found while investigating https://github.com/edx/edx-platform/pull/23039#discussion_r417670046

This PR does:

  • provide example of html escaping for the interpolateHtml method
  • fix misleading comments about escaping for StringUtils.interpolate (it
    does not in fact escape anything)
  • update interpolate examples for accuracy and to better represent functionality
  • add a test to show that html is not escaped

Jira tickets: OSPR-4435

Test instructions:

  • cross reference the updated doc comments with the code, tests, and official docs to verify accuracy
  • verify that tests pass

Reviewers:

@openedx-webhooks
Copy link

Thanks for the pull request, @swalladge! I've created OSPR-4435 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Apr 30, 2020
@samuelallan72 samuelallan72 mentioned this pull request Apr 30, 2020
Merged
2 tasks
@natabene
Copy link

@swalladge Thank you for your contribution. Please let me know once it is ready for our review.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Apr 30, 2020
bradenmacdonald
bradenmacdonald suggested changes Apr 30, 2020
View reviewed changes
src/js/utils/string-utils.js Outdated Show resolved Hide resolved
robrap
robrap reviewed Apr 30, 2020
View reviewed changes
src/js/utils/html-utils.js Outdated Show resolved Hide resolved
src/js/utils/html-utils.js Outdated Show resolved Hide resolved
src/js/utils/string-utils.js Outdated Show resolved Hide resolved
@openedx-webhooks openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 1, 2020
robrap
robrap reviewed May 1, 2020
View reviewed changes
src/js/utils/string-utils.js Outdated Show resolved Hide resolved
@bradenmacdonald
Copy link

👍 LGTM FWIW though I'm not super familiar with this area.

robrap
robrap approved these changes May 4, 2020
View reviewed changes
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

One minor change requested. Thanks.

src/js/utils/string-utils.js Outdated Show resolved Hide resolved
@natabene
Copy link

natabene commented May 6, 2020

@robrap Can you give another look when you have a chance?

robrap
robrap approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Can merge as soon as it is green.

src/js/utils/specs/string-utils-spec.js Outdated
@@ -27,6 +27,10 @@ define(
'Hello, {name}. Here is a { followed by a }', {name: 'Andy'},
'Hello, Andy. Here is a { followed by a }'
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a comma.
Maybe this is breaking the build? The error reported in Travis is not helpful. I re-ran master and it is passing, so hopefully this is it.

Copy link
Contributor

@robrap robrap May 7, 2020 •
edited

Choose a reason for hiding this comment

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

@swalladge: Maybe you missed the first line above? There is a bug. You are missing a comma at the end of line 29.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, nice catch.

The unhelpful travis error is due to recent changes with how xvfb works with travis (the previous builds including the one you reran were on trusty, travis now defaults to xenial). I'm going to try using the latest method for xvfb with xenial, otherwise will explicitly pin back to trusty and use the original method.

Copy link
Author

Choose a reason for hiding this comment

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

Tests passing now, i'll squash in prep for merge. Thanks for your review @robrap ! :)

Update interpolate doc comments for accuracy
863cdd1 
- provide example of html escaping for the interpolateHtml method
- fix misleading comments about escaping for StringUtils.interpolate (it
  does not in fact escape anything)
- update interpolate examples for accuracy and to better represent functionality
- pin travis image to trusty - current setup is not compatible with
  latest default image (xenial)
@robrap robrap merged commit cd81af9 into openedx:master May 8, 2020
@openedx-webhooks
Copy link

@swalladge 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@samuelallan72 samuelallan72 deleted the samuel/comment-accuracy branch May 9, 2020 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robrap robrap robrap approved these changes

@bradenmacdonald bradenmacdonald bradenmacdonald approved these changes

Assignees
No one assigned
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@samuelallan72 @openedx-webhooks @natabene @bradenmacdonald @robrap