Skip to content

Remove extra space before semicolon and remove extra comma in php, phtml and js files #26342

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.

Sign up for GitHub

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

Jump to bottom
Merged
magento-engcom-team merged 4 commits into from
Feb 13, 2020
Merged

Remove extra space before semicolon and remove extra comma in php, phtml and js files #26342

magento-engcom-team merged 4 commits into from
Feb 13, 2020

Conversation

@tejash-wagento
Copy link
Contributor

@tejash-wagento tejash-wagento commented Jan 10, 2020 •
edited
Loading

Description

Remove extra space before semicolon and remove extra comma in php, phtml and js files

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Jan 10, 2020

Hi @tejash-wagento. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@magento-engcom-team magento-engcom-team added Partner: Wagento Pull Request is created by partner Wagento partners-contribution Pull Request is created by Magento Partner labels Jan 10, 2020
@ghost ghost assigned dmytro-ch Jan 10, 2020
@dmytro-ch dmytro-ch added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Cleanup labels Jan 10, 2020
@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Jan 10, 2020

Hi @dmytro-ch, thank you for the review.
ENGCOM-6580 has been created to process this Pull Request
✳️ @dmytro-ch, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

orlangur
orlangur requested changes Jan 10, 2020
View reviewed changes
app/code/Magento/Quote/Test/Unit/Model/Product/Plugin/UpdateQuoteItemsTest.php
[10, 20, 'once'],
[null, 10, 'never'],
[10, 10, 'never'],
[10, 10, 'once', true],
Copy link
Contributor

@orlangur orlangur Jan 10, 2020

Choose a reason for hiding this comment

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

This change is definitely not needed

Copy link
Contributor Author

@tejash-wagento tejash-wagento Jan 13, 2020

Choose a reason for hiding this comment

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

added comma after array

app/code/Magento/AdminAnalytics/view/adminhtml/templates/notification.phtml
define('analyticsPopupConfig', function () {
return {
analyticsVisible: <?= $block->getNotification()->isAnalyticsVisible() ? 1 : 0; ?>,
releaseVisible: <?= $block->getNotification()->isReleaseVisible() ? 1 : 0; ?>,
Copy link
Contributor

@orlangur orlangur Jan 10, 2020

Choose a reason for hiding this comment

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

This change is not needed if it works with trailing comma.

Copy link
Contributor

@hostep hostep Jan 11, 2020

Choose a reason for hiding this comment

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

I agree with @orlangur here, keeping comma's in javascript objects or php arrays is a good thing, it gives a clearer diff in git when one or multiple lines are added if a comma already existed on the line above the new line.

For javascript, historically this was an issue in older Internet Explorer versions, they couldn't handle that, but according to this comment it was fixed in IE9 and higher and Magento only supports IE11, so we should be fine.

PHP is more and more encouraging this kind of syntax btw, PHP 7.3 got a new feature where arguments in function/method calls can now also have a trailing comma: https://www.php.net/manual/en/migration73.new-features.php#migration73.new-features.core.trailing-commas

Copy link
Contributor Author

@tejash-wagento tejash-wagento Jan 13, 2020

Choose a reason for hiding this comment

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

@orlangur, comma added after array,
Thanks for suggestion.

@ghost ghost assigned orlangur Jan 10, 2020
dmytro-ch
dmytro-ch requested changes Jan 11, 2020
View reviewed changes
Copy link
Contributor

@dmytro-ch dmytro-ch left a comment

Choose a reason for hiding this comment

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

@tejash-wagento, could you please also check and fix the static test failures?

@tejash-wagento
Copy link
Contributor Author

tejash-wagento commented Jan 13, 2020

@dmytro-ch, issue fixed

@engcom-Golf engcom-Golf self-assigned this Feb 7, 2020
@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Feb 10, 2020

Hi @dmytro-ch, thank you for the review.
ENGCOM-6580 has been created to process this Pull Request

@engcom-Delta engcom-Delta self-assigned this Feb 11, 2020
@engcom-Delta
Copy link
Contributor

engcom-Delta commented Feb 11, 2020

Notice: QA not applicable

magento-engcom-team pushed a commit that referenced this pull request Feb 13, 2020
@slavvka
ENGCOM-6580: Remove extra space before semicolon and remove extra com…
90bf25a 
…ma in php, phtml and js files #26342
@magento-engcom-team magento-engcom-team merged commit 9d992d0 into magento:2.4-develop Feb 13, 2020
@m2-assistant
Copy link

m2-assistant bot commented Feb 13, 2020

Hi @tejash-wagento, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@orlangur orlangur orlangur requested changes

@dmytro-ch dmytro-ch dmytro-ch approved these changes

@DrewML DrewML Awaiting requested review from DrewML

@YevSent YevSent Awaiting requested review from YevSent

@paliarush paliarush Awaiting requested review from paliarush

+1 more reviewer

@hostep hostep hostep left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@orlangur orlangur

@dmytro-ch dmytro-ch

@engcom-Delta engcom-Delta

@engcom-Golf engcom-Golf

Labels

Area: Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Cleanup Component: Backend Component: CatalogGraphQl Component: Csp Component: Payment Component: Quote Component: Sales Component: Tinymce Partner: Wagento Pull Request is created by partner Wagento partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@tejash-wagento @magento-engcom-team @engcom-Delta @hostep @orlangur @dmytro-ch @engcom-Echo @engcom-Golf @Nazar65