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

Use Dict.getArray, instead of Dict.get, when getting the 'Size' in constructSampled in src/core/function.js #9740

Merged
merged 1 commit into from Jun 2, 2018

Conversation

ptmxyz
Copy link

@ptmxyz ptmxyz commented May 19, 2018

Use Dict.getArray, instead of Dict.get, when getting the 'Size' in constructSampled on line 189 of src/core/function.js

Fixes #9734.

@Snuffleupagus
Copy link
Collaborator

Drive by comment: Please provide a better commit message! Reading just the commit message, and not the code, it isn't really possible to understand the change. Furthermore the use of the word "last" isn't correct, since there's a number of Dict.get occurences in that file (but those should definitely not be changed).

A better suggestion might be e.g:

Use Dict.getArray, instead of Dict.get, when getting the 'Size' in constructSampled in src/core/function.js

@ptmxyz ptmxyz changed the title replaced last dict.get with dict.getArray Replaced dict.get with dict.getArray in one spot in functions.js May 20, 2018
@ptmxyz
Copy link
Author

ptmxyz commented May 20, 2018

@Snuffleupagus thanks for the feedback! I updated my commit message and title accordingly.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented May 21, 2018

I updated my commit message and title accordingly.

It doesn't look like the commit message was updated, only the PR title; did you forget to force push your changes with git push -f ...?
Besides there's now a typo in the PR title, since there's no file called "functions.js" (note the extra "s"). Furthermore, it's still unnecessarily imprecise, so why not simply use the suggestion above verbatim!?

Since you'll need to actually push the changes anyway, please update the commit message to:
"Use Dict.getArray, instead of Dict.get, when getting the 'Size' in constructSampled in src/core/function.js (PR 7295 follow-up)"

@timvandermeij
Copy link
Contributor

@pedrotp Hi, did you manage to update the commit message so we can try to merge it? You can do this locally with git commit --amend, entering the new commit message as outlined above and then using git push -f to push the new commit. Thank you!

…nstructSampled in src/core/function.js (PR 7295 follow-up)
@ptmxyz ptmxyz changed the title Replaced dict.get with dict.getArray in one spot in functions.js Use Dict.getArray, instead of Dict.get, when getting the 'Size' in constructSampled in src/core/function.js Jun 2, 2018
@ptmxyz
Copy link
Author

ptmxyz commented Jun 2, 2018

@timvandermeij @Snuffleupagus sorry, first commit. edited the commit message, description and title, should be good to go now. thanks again for the feedback.

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jun 2, 2018

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/21faef71b7d9230/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 2, 2018

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/1d59f23276c8253/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jun 2, 2018

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/21faef71b7d9230/output.txt

Total script time: 19.01 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Jun 2, 2018

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/1d59f23276c8253/output.txt

Total script time: 24.56 mins

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

Image differences available at: http://54.215.176.217:8877/1d59f23276c8253/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit 36af85d into mozilla:master Jun 2, 2018
@timvandermeij
Copy link
Contributor

Thank you for your contribution!

(Note that the failing tests on the Windows bot are known intermittent failures, so they are unrelated to this patch.)

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Use Dict.getArray, instead of Dict.get, when getting the 'Size' in constructSampled in src/core/function.js
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.

None yet

4 participants