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

Fix#7378 - Changing accumulator variable names #17915

Closed
wants to merge 3 commits into from
Closed

Fix#7378 - Changing accumulator variable names #17915

wants to merge 3 commits into from

Conversation

jpjjulie
Copy link

@jpjjulie jpjjulie commented Oct 11, 2019 •
edited by gziolo
Loading

Description

Fixes #7378.

Changed accumulator variable names

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Code Quality Issues or PRs that relate to code quality Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Oct 15, 2019
gziolo
gziolo reviewed Oct 15, 2019
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Nice improvements. Can we address two more comments where I suggest we remove redundant prefixes result and memo from the variable names? They are the same I shared in #17893 😄

Otherwise, it looks great. Many thanks for cleaning up the code.

packages/block-editor/src/store/reducer.js Outdated
@@ -67,8 +67,8 @@ function mapBlockOrder( blocks, rootClientId = '' ) {
* @return {Object} Block order map object.
*/
function mapBlockParents( blocks, rootClientId = '' ) {
return blocks.reduce( ( result, block ) => Object.assign(
result,
return blocks.reduce( ( resultAccumulator, block ) => Object.assign(
Copy link
Member

Choose a reason for hiding this comment

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

It seems like resultAccumulator could be simplified to just accumulator. They result prefix doesn't sound like it brings additional context here.

packages/data/src/store/index.js Outdated
@@ -16,8 +16,8 @@ function createCoreDataStore( registry ) {
'hasFinishedResolution',
'isResolving',
'getCachedResolvers',
].reduce( ( memo, selectorName ) => ( {
...memo,
].reduce( ( memoAccumulator, selectorName ) => ( {
Copy link
Member

Choose a reason for hiding this comment

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

A similar note here. It feels like memoAccumulator could be shortened to accumulator and it will read well enough.

@gziolo
Copy link
Member

gziolo commented Oct 15, 2019

I have just merged #17893 from @lozinska and it looks like you both applied changes to packages/block-library/src/group/index.js. It'd be nice to rebase this branch with the latest changes from master (see https://github.com/WordPress/gutenberg/blob/master/docs/contributors/git-workflow.md#keeping-your-fork-up-to-date).

@jpjjulie
Copy link
Author

@gziolo I will work on this. Thanks.

WunderBart
WunderBart reviewed Mar 23, 2020
View reviewed changes
Copy link
Member

@WunderBart WunderBart left a comment

Choose a reason for hiding this comment

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

Just 2 more files to update and some conflicts to resolve and we're ready to go! 😄

packages/core-data/src/index.js
@@ -19,26 +19,26 @@ import { REDUCER_KEY } from './name';
// Instead of getEntityRecord, the consumer could use more user-frieldly named selector: getPostType, getTaxonomy...
// The "kind" and the "name" of the entity are combined to generate these shortcuts.

const entitySelectors = defaultEntities.reduce( ( result, entity ) => {
const entitySelectors = defaultEntities.reduce( ( resultAccumulator, entity ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have those renamed to accumulator too?

webpack.config.js
@@ -33,10 +33,10 @@ const gutenbergPackages = Object.keys( dependencies )

module.exports = {
mode,
entry: gutenbergPackages.reduce( ( memo, packageName ) => {
entry: gutenbergPackages.reduce( ( memoAccumulator, packageName ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above! 🙏

@guarani
Copy link
Contributor

guarani commented Oct 27, 2020

Looks like this needs to be rebased with master and conflicts resolved to be testable on mobile since this PR pre-dates the monorepo changes.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Nov 28, 2020
Base automatically changed from master to trunk March 1, 2021 15:42
@guarani guarani removed their request for review January 19, 2022 13:00
@gziolo
Copy link
Member

gziolo commented May 7, 2022

This one got completely out of date is no longer actionable. Thank you so much for working on it. We can always reopen when this branch gets refreshed.

@gziolo gziolo closed this May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WunderBart WunderBart WunderBart left review comments

@gziolo gziolo gziolo left review comments

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

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

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

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

@oandregal oandregal Awaiting requested review from oandregal

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

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set a consistent and readable variable name for Array#reduce accumulator variable
4 participants
@jpjjulie @gziolo @guarani @WunderBart