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.

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

CS/QA: various minor fixes #44551

Merged
merged 5 commits into from
Oct 3, 2022
Merged

CS/QA: various minor fixes #44551

merged 5 commits into from
Oct 3, 2022

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Sep 28, 2022

What?

Various small CS/QA fixes.

Why?

To improve compatibility/mergability with WP Core.

How?

  • Minor whitespace fix.
    Removes a space in a place where there shouldn't be a space.
  • Remove redundant blank lines at the end of functions.
  • Remove redundant semi-colon.
    The semi-colon effectively created a new, empty PHP statement, where none was needed.
  • Use in/decrementors where appropriate.
    Using in/decrementors is more performant than an assignment with a calculation.
  • Remove redundant concatenation.
    Every concatenation exponentially increases the memory needed for the string in PHP, so this removes unnecessary concatenations, i.e. two hard-coded strings being concatenated.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 28, 2022
@jrfnl jrfnl removed the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 28, 2022
@WordPress WordPress deleted a comment from github-actions bot Sep 28, 2022
michalczaplinski
michalczaplinski approved these changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Just for my understanding - in what way does it help mergeability with core? Are the coding styles somehow incompatible between Gutenberg and Core?

@jrfnl
Copy link
Member Author

jrfnl commented Sep 29, 2022

Just for my understanding - in what way does it help mergeability with core? Are the coding styles somehow incompatible between Gutenberg and Core?

There are a number of standards for Core, which are applied in Core, but aren't actively enforced via tooling at this time.
PRs from GB still need to comply with those type of standards for merges with Core, otherwise things will need to be tidied up afterwards again.

Note: as of the upcoming WPCS 3.0.0 a lot of those standards which are currently not (yet) enforced, will start to get enforced, so this also sets the GB project up to be more prepared for WPCS 3.0.0.

@jrfnl
Copy link
Member Author

jrfnl commented Sep 29, 2022

@michalczaplinski I notice the PR shows a conflict by now - what is the convention for handling this in this repo ? I would normally do a local rebase without additional changes and force push to the branch. Is that okay ?

@michalczaplinski
Copy link
Contributor

@michalczaplinski I notice the PR shows a conflict by now - what is the convention for handling this in this repo ? I would normally do a local rebase without additional changes and force push to the branch. Is that okay ?

Yeah, that's fine! The commits get squashed anyway when you merge your PR 🙂

Thanks for your explanation about coding styles, makes sense to me.

A complete aside: In the long term, I would love to see those kind of changes enforced via tooling. I'd be happy with whatever code style as long as the code can be formatted automatically.

@jrfnl
Copy link
Member Author

jrfnl commented Sep 29, 2022 •
edited
Loading

A complete aside: In the long term, I would love to see those kind of changes enforced via tooling.

And that's exactly what WPCS will start to do. ;-) (as of version 3.0.0)

@jrfnl jrfnl force-pushed the CSQA/various-minor-fixes branch from 7ef97a2 to fff668c Compare September 29, 2022 18:40
@jrfnl
Copy link
Member Author

jrfnl commented Sep 29, 2022

Rebased without changes to get past an imaginary merge conflict.

anton-vlasenko
anton-vlasenko approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!
Good job.
P.S. There is one failing e2e test, but I guess it is unrelated to the fixes in this PR. I've restarted the CI checks. Hopefully, the error will gone.

@anton-vlasenko anton-vlasenko added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 30, 2022
@anton-vlasenko anton-vlasenko added this to the Gutenberg 14.3 milestone Sep 30, 2022
@anton-vlasenko anton-vlasenko force-pushed the CSQA/various-minor-fixes branch from fff668c to ffa8cf9 Compare September 30, 2022 20:12
@anton-vlasenko
Copy link
Contributor

I rebased this PR last Friday and it fixed the issue with the e2e test.

@aristath aristath merged commit 303c6ad into trunk Oct 3, 2022
@aristath aristath deleted the CSQA/various-minor-fixes branch October 3, 2022 14:23
This was referenced Jan 17, 2023
Closed
Closed
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Jan 25, 2023
@hellofromtonya
Editor: Use in/decrementors in wp_tinycolor_hue_to_rgb().
616f0bc 
Replaces `+=` and `-=` with `++$t` and `--$t` (in/decrementors) within the `wp_tinycolor_hue_to_rgb()`. 

Why? For performance: in/decrementors are more performant than an assignment with a calculation.

References:
* https://www.php.net/manual/en/language.operators.increment.php
* WordPress/gutenberg#44551

Follow-up to [50929].

Props jrf, czapla, antonvlasenko, aristath, mamaduka.
See #57527. 

git-svn-id: https://develop.svn.wordpress.org/trunk@55140 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jan 25, 2023
@hellofromtonya
Editor: Use in/decrementors in wp_tinycolor_hue_to_rgb().
e9a172b 
Replaces `+=` and `-=` with `++$t` and `--$t` (in/decrementors) within the `wp_tinycolor_hue_to_rgb()`. 

Why? For performance: in/decrementors are more performant than an assignment with a calculation.

References:
* https://www.php.net/manual/en/language.operators.increment.php
* WordPress/gutenberg#44551

Follow-up to [50929].

Props jrf, czapla, antonvlasenko, aristath, mamaduka.
See #57527. 
Built from https://develop.svn.wordpress.org/trunk@55140


git-svn-id: http://core.svn.wordpress.org/trunk@54673 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Jan 25, 2023
@hellofromtonya
Editor: Use in/decrementors in wp_tinycolor_hue_to_rgb().
c5b1447 
Replaces `+=` and `-=` with `++$t` and `--$t` (in/decrementors) within the `wp_tinycolor_hue_to_rgb()`. 

Why? For performance: in/decrementors are more performant than an assignment with a calculation.

References:
* https://www.php.net/manual/en/language.operators.increment.php
* WordPress/gutenberg#44551

Follow-up to [50929].

Props jrf, czapla, antonvlasenko, aristath, mamaduka.
See #57527. 
Built from https://develop.svn.wordpress.org/trunk@55140


git-svn-id: https://core.svn.wordpress.org/trunk@54673 1a063a9b-81f0-0310-95a4-ce76da25c4cd
VenusPR added a commit to VenusPR/Wordpress_Richard that referenced this pull request Mar 9, 2023
@VenusPR
Editor: Use in/decrementors in wp_tinycolor_hue_to_rgb().
c057948 
Replaces `+=` and `-=` with `++$t` and `--$t` (in/decrementors) within the `wp_tinycolor_hue_to_rgb()`. 

Why? For performance: in/decrementors are more performant than an assignment with a calculation.

References:
* https://www.php.net/manual/en/language.operators.increment.php
* WordPress/gutenberg#44551

Follow-up to [50929].

Props jrf, czapla, antonvlasenko, aristath, mamaduka.
See #57527. 
Built from https://develop.svn.wordpress.org/trunk@55140


git-svn-id: http://core.svn.wordpress.org/trunk@54673 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anton-vlasenko anton-vlasenko anton-vlasenko approved these changes

@michalczaplinski michalczaplinski michalczaplinski approved these changes

@aristath aristath aristath approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees
No one assigned
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 14.3
Development

Successfully merging this pull request may close these issues.
4 participants
@jrfnl @michalczaplinski @anton-vlasenko @aristath