Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Remove redundant sprintf argument.#24613
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| returnsprintf('($context["%s"] ?? $this->getContext($context, "%s"))',$name,$name,$name); | ||
| returnsprintf('($context["%s"] ?? $this->getContext($context, "%s"))',$name,$name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
%1$s? thus assign$name once here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't you find that notation (%1$s) difficult to understand? I always struggle with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No strong opinion. Just to make things less error prone :) basically the two assignments must always stay the same, thus id prefer a single assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL I thought about it, but decided not to change. But I agree that%1$s is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I prefer%1$s. That's howsprintf() should be used to avoid duplications, and we already have some usages in the code base
Tobion commentedOct 19, 2017
This PR should be opened against 2.7 as it is wrong there as well. |
Aliance commentedOct 19, 2017
@Tobion there is different code in different branches: So in some branches there is php version checks, in others – not. May be merge this pr to branches w/o php version checks and create another one for other branches? |
Tobion commentedOct 19, 2017
@Aliance it's enough if you open a PR for 2.7. We then merge it upwards and will resolve conflicts. |
0b88aed toc8012f0CompareAliance commentedOct 20, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Tobion I rebased it on 2.7, also changed duplicated arguments to |
| $formId = Crawler::xpathLiteral($this->node->getAttribute('id')); | ||
| $fieldNodes =$xpath->query(sprintf('descendant::input[@form=%s] | descendant::button[@form=%s] | descendant::textarea[@form=%s] | descendant::select[@form=%s] | //form[@id=%s]//input[not(@form)] | //form[@id=%s]//button[not(@form)] | //form[@id=%s]//textarea[not(@form)] | //form[@id=%s]//select[not(@form)]',$formId,$formId,$formId,$formId,$formId,$formId,$formId,$formId)); | ||
| $fieldNodes =$xpath->query(sprintf('descendant::input[@form=%s] | descendant::button[@form=%1$s] | descendant::textarea[@form=%1$s] | descendant::select[@form=%1$s] | //form[@id=%1$s]//input[not(@form)] | //form[@id=%1$s]//button[not(@form)] | //form[@id=%1$s]//textarea[not(@form)] | //form[@id=%1$s]//select[not(@form)]',$formId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lol :) definitely up for%1$s now.
Tobion commentedOct 26, 2017
Good catch, thanks@Aliance. |
This PR was merged into the 2.7 branch.Discussion----------Remove redundant sprintf argument.| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22820| License | MIT| Doc PR | –cc@fabpot@ogizanagiCommits-------c8012f0 Remove redundant sprintf arguments.
Uh oh!
There was an error while loading.Please reload this page.
cc@fabpot@ogizanagi