Skip to content

Style of acceptance test string concatenation#32336

Merged
phil-davis merged 4 commits into
masterfrom
acceptance-test-code-style-20180814
Aug 21, 2018
Merged

Style of acceptance test string concatenation#32336
phil-davis merged 4 commits into
masterfrom
acceptance-test-code-style-20180814

Conversation

@phil-davis

@phil-davis phil-davis commented Aug 14, 2018

Copy link
Copy Markdown
Contributor

Description

Adjust the style of string concatenation in places that I found.

Motivation and Context

Review comment #29773 (comment)

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@codecov

codecov Bot commented Aug 14, 2018

Copy link
Copy Markdown

Codecov Report

Merging #32336 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #32336   +/-   ##
=========================================
  Coverage     64.01%   64.01%           
  Complexity    18564    18564           
=========================================
  Files          1171     1171           
  Lines         69851    69851           
  Branches       1267     1267           
=========================================
  Hits          44714    44714           
  Misses        24767    24767           
  Partials        370      370
Flag Coverage Δ Complexity Δ
#javascript 52.84% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.28% <ø> (ø) 18564 <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 016f1e0...afcda1c. Read the comment docs.

@phil-davis phil-davis force-pushed the acceptance-test-code-style-20180814 branch from 5475cfc to afcda1c Compare August 15, 2018 03:38
@phil-davis

Copy link
Copy Markdown
Contributor Author

@individual-it @DeepDiver1975 please comment - what is the project "standard" for forming strings in PHP?

@DeepDiver1975

Copy link
Copy Markdown
Member

Benefits in Performance and memory consumption are to be ignored when comparing string concat vs string Interpolation.
I find Interpolation nicer to read.

@phil-davis

phil-davis commented Aug 15, 2018

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975 do the style of changes I made here match the style you would like to have?
If yes, then approve this PR.
If no, then mention what could be different (e.g. maybe I went too far putting some things inline with "folder/{$arrayXyz['key']}/stuff" type of syntax)

@phil-davis

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975 @individual-it comments or approval please.

@individual-it individual-it left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like concatenation more, but lets not get blocked by that and just define this as standard and get on with work

@phil-davis phil-davis merged commit ef09c78 into master Aug 21, 2018
@phil-davis phil-davis deleted the acceptance-test-code-style-20180814 branch August 21, 2018 09:28
@phil-davis

Copy link
Copy Markdown
Contributor Author

Backport stable10 #32393

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants