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

Truncate debug output for long data URLs #22485 #22740

Merged
merged 3 commits into from Jan 28, 2019

Conversation

Projects
None yet
5 participants
@Hyperion101010
Copy link
Contributor

Hyperion101010 commented Jan 21, 2019

I tried to solve the patch, here i did what you told but i am not getting the
exact result after the build can you take a look


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22485 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 21, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon.

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jan 21, 2019

@jdm r?

@paulrouget paulrouget assigned jdm and unassigned paulrouget Jan 21, 2019

@jdm
Copy link
Member

jdm left a comment

Good start!

Show resolved Hide resolved components/url/lib.rs Outdated
Show resolved Hide resolved components/url/lib.rs Outdated
Show resolved Hide resolved components/url/lib.rs Outdated
Show resolved Hide resolved components/url/lib.rs Outdated
@jdm
Copy link
Member

jdm left a comment

Don't forget to run ./mach fmt to update your changes to match the official code style!

Show resolved Hide resolved components/url/lib.rs Outdated
Show resolved Hide resolved components/url/lib.rs Outdated
Show resolved Hide resolved components/url/lib.rs Outdated

@Hyperion101010 Hyperion101010 force-pushed the Hyperion101010:master branch from 687a84a to b2c9b1c Jan 21, 2019

Show resolved Hide resolved components/url/lib.rs Outdated
Show resolved Hide resolved components/url/lib.rs Outdated
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 21, 2019

Also, let's use a commit message that is more meaningful - "Truncate long URLs when doing debug prints."

@Hyperion101010 Hyperion101010 force-pushed the Hyperion101010:master branch from b2c9b1c to e916877 Jan 21, 2019

@Hyperion101010 Hyperion101010 force-pushed the Hyperion101010:master branch from e916877 to 5170b06 Jan 21, 2019

Show resolved Hide resolved components/url/lib.rs Outdated
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 22, 2019

Looks like we need to find some pieces of code that are using the Debug string representation for a URL and make them use to_string instead.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 23, 2019

My mistake; this is caused by the changes being made to the fmt::Display impl, rather than fmt::Debug. If we make that change instead, I would expect the tests to pass again.

@Hyperion101010 Hyperion101010 force-pushed the Hyperion101010:master branch from 6988ab4 to d4dc117 Jan 25, 2019

@Hyperion101010

This comment has been minimized.

Copy link
Contributor Author

Hyperion101010 commented Jan 26, 2019

This looks good, all checks are passed !

Truncate long URLs when doing debug prints
  I added the required changes on debug in lib.rs file, you can have a review

@Hyperion101010 Hyperion101010 force-pushed the Hyperion101010:master branch from d4dc117 to abc5b2a Jan 28, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 28, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 28, 2019

📌 Commit abc5b2a has been approved by jdm

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 28, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 28, 2019

📌 Commit 21e10b2 has been approved by jdm

bors-servo added a commit that referenced this pull request Jan 28, 2019

Auto merge of #22740 - Hyperion101010:master, r=jdm
  Truncate debug output for long data URLs #22485

  I tried to solve the patch, here i did what you told but i am not getting the
  exact result after the build can you take a look

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #22485 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [x] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22740)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 28, 2019

⌛️ Testing commit 21e10b2 with merge 07d53e3...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 28, 2019

@bors-servo bors-servo merged commit 21e10b2 into servo:master Jan 28, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment