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

Fix lint warnings in URL polyfill #7626

Merged
merged 1 commit into from
Sep 12, 2016

Conversation

yashsriv
Copy link
Contributor

@yashsriv yashsriv commented Sep 12, 2016

Fixes #6872.

@yashsriv
Copy link
Contributor Author

I have a doubt regarding the object which was called jURL earlier. JSHint was throwing an error regarding UpperCamelCase for Constructor names so I chose JURL but it doesn't look very nice.

P.S. This is my first P.R. on any open source project ever. Please suggest changes if any.

if (null !== this._password) {
this._password += tempC;
} else {
this._username += tempC;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove two spaces here.

@timvandermeij
Copy link
Contributor

timvandermeij commented Sep 12, 2016

Thank you for your contribution! Your patch looks good and I have added a few comments to improve it. JURL seems fine to me. Linting does pass, so that's nice! When you address these comments, please squash the commits into one afterwards (see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits on how to do this easily). If you have any questions, feel free to ask!

Note that I adjusted the title of the pull request to make it more descriptive. I also put "Fixes #..." in the pull request description so that GitHub closes the issue automatically once we merge this pull request. If possible, after squashing the commits, change the commit message to "Fix lint warnings in URL polyfill" (you can do this with git commit --amend for example).

@timvandermeij timvandermeij changed the title Addresses #6872 Fix lint warnings in URL polyfill Sep 12, 2016
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Sep 12, 2016

Also it'd be great if the commit message was updated to e.g. Fix lint warnings in URL polyfill or similar, since the current Addresses #6872 - Fixed linting for the file src/shared/util.js doesn't really give too much information about what changed and why!

@yashsriv
Copy link
Contributor Author

Updated as per the comments.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/69747ff130df57b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/69747ff130df57b/output.txt

Total script time: 1.18 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/2b222e7f2db225e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/25184ab8769851f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/2b222e7f2db225e/output.txt

Total script time: 24.72 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/25184ab8769851f/output.txt

Total script time: 37.81 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@timvandermeij timvandermeij merged commit 4d15928 into mozilla:master Sep 12, 2016
@timvandermeij
Copy link
Contributor

Nice work, thank you!

@yashsriv yashsriv deleted the lint-fix-url-polyfill branch September 12, 2016 19:17
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants