Skip to content

[Due for payment 2025-04-14] Update style guide to require parameter objects for functions with many parameters #57260

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
Closed
neil-marcellini opened this issue Feb 21, 2025 · 24 comments
Closed

[Due for payment 2025-04-14] Update style guide to require parameter objects for functions with many parameters #57260

neil-marcellini opened this issue Feb 21, 2025 · 24 comments
Assignees
@slafortune @sonialiap @neil-marcellini @Krishna2323
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Improvement Item broken or needs improvement.

Comments

@neil-marcellini
Copy link
Contributor

neil-marcellini commented Feb 21, 2025 •
edited
Loading

Problem

In the parent tracking issue we identified a problem when functions have too many parameters, and we're working to refactor this. However, it's not enforced by our style guide yet. We need to come up with some recommendations and rules about this so we can stay consistent once the refactor is done.

Solution

Start a conversation in Slack with suggestions about what the style guide should say, then implement it for App. We can also consider whether we want to do something similar on the backend.

Issue OwnerCurrent Issue Owner: @sonialiap
@neil-marcellini neil-marcellini added Improvement Item broken or needs improvement. Weekly KSv2 labels Feb 21, 2025
@neil-marcellini neil-marcellini self-assigned this Feb 21, 2025
@Krishna2323
Copy link
Contributor

@neil-marcellini for frontend, I think we can add '@typescript-eslint/max-params': ['error', {max: 5}], in .eslintrc.js and that would be enough. The error message looks like:

//Users/krishna2323/Desktop/workspace/expensify/App/src/pages/workspace/invoices/WorkspaceInvoiceVBASection.tsx
  92:7  error  Arrow function has too many parameters (7). Maximum allowed is 5  @typescript-eslint/max-params

//Users/krishna2323/Desktop/workspace/expensify/App/src/styles/utils/index.ts
  367:1  error  Function 'getZoomSizingStyle' has too many parameters (7). Maximum allowed is 5          @typescript-eslint/max-params
  515:1  error  Function 'getIconWidthAndHeightStyle' has too many parameters (6). Maximum allowed is 5  @typescript-eslint/max-params
  529:1  error  Function 'getButtonStyleWithIcon' has too many parameters (7). Maximum allowed is 5      @typescript-eslint/max-params

The following functions have more than 5 params:

Functions with more than 5 params - Total: 115 functions

📂 src/libs/actions/IOU.ts

  1. getTrackExpenseInformation - 21 parameters (line 3079)
  2. buildOnyxDataForInvoice - 19 parameters (line 1464)
  3. buildOnyxDataForTrackExpense - 14 parameters (line 1862)
  4. createSplitsAndOnyxData - 16 parameters (line 5048)
  5. getSendInvoiceInformation - 9 parameters (line 2472)
  6. updateMoneyRequestAttendees - 7 parameters (line 3935)
  7. updateMoneyRequestTag - 7 parameters (line 3953)
  8. updateMoneyRequestCategory - 7 parameters (line 4085)
  9. updateMoneyRequestDistanceRate - 8 parameters (line 4127)
  10. sendInvoice - 9 parameters (line 4708)
  11. getSendMoneyParams - 7 parameters (line 7146)
  12. getPayMoneyRequestParams - 6 parameters (line 7747)
  13. sendMoneyElsewhere - 6 parameters (line 8039)
  14. sendMoneyWithWallet - 6 parameters (line 8052)
  15. canIOUBePaid - 8 parameters (line 8100)
  16. navigateToStartStepIfScanFileCannotBeRead - 8 parameters (line 9521)

📂 src/libs/ReportUtils.ts

  1. buildOptimisticChatReport - 15 parameters (line 6015)
  2. buildOptimisticMoneyRequestEntities - 15 parameters (line 6788)
  3. buildOptimisticIOUReportAction - 13 parameters (line 5462)
  4. getIcons - 7 parameters (line 2635)
  5. getReportPreviewMessage - 7 parameters (line 3892)
  6. buildOptimisticAddCommentReportAction - 6 parameters (line 4864)
  7. buildOptimisticTaskCommentReportAction - 7 parameters (line 4974)
  8. buildOptimisticIOUReport - 7 parameters (line 5053)
  9. buildOptimisticInvoiceReport - 6 parameters (line 5129)
  10. buildOptimisticExpenseReport - 7 parameters (line 5207)
  11. getIOUReportActionMessage - 7 parameters (line 5385)
  12. buildOptimisticReportPreview - 6 parameters (line 5699)
  13. buildOptimisticModifiedExpenseReportAction - 6 parameters (line 5782)
  14. buildOptimisticTaskReport - 7 parameters (line 6669)
  15. getTaskAssigneeChatOnyxData - 7 parameters (line 7990)

📂 src/libs/actions/Report.ts

  1. completeOnboarding - 10 parameters (line 4196)
  2. openReport - 8 parameters (line 846)
  3. navigateToAndOpenReport - 7 parameters (line 1145)
  4. toggleEmojiReaction - 6 parameters (line 2847)
  5. prepareOnboardingOnyxData - 6 parameters (line 3635)

📂 src/libs/actions/Policy/Policy.ts

  1. buildPolicyData - 9 parameters (line 1731)
  2. createWorkspace - 8 parameters (line 2042)
  3. createDraftInitialWorkspace - 6 parameters (line 1665)
  4. createDraftWorkspace - 6 parameters (line 2081)

📂 src/libs/actions/Task.ts

  1. createTaskAndNavigate - 8 parameters (line 116)
  2. setAssigneeValue - 6 parameters (line 833)

📂 src/libs/actions/App.ts

  1. createWorkspaceWithPolicyDraftAndNavigateToIt - 9 parameters (line 416)
  2. savePolicyDraftByNewWorkspace - 6 parameters (line 452)

📂 src/libs/actions/PersonalDetails.ts

  1. updateAddress - 6 parameters (line 213)

📂 src/libs/actions/CompanyCards.ts

  1. setCompanyCardExportAccount - 6 parameters (line 584)

📂 src/libs/actions/Card.ts

  1. updateExpensifyCardLimit - 6 parameters (line 407)

📂 src/libs/actions/EmojiPickerAction.ts

  1. showEmojiPicker - 7 parameters (line 52)

📂 src/libs/actions/connections/NetSuiteCommands.ts

  1. updateNetSuiteSyncOptionsOnyxData - 6 parameters (line 138)

📂 src/libs/actions/connections/SageIntacct.ts

  1. prepareOnyxDataForUserDimensionUpdate - 6 parameters (line 238)

📂 src/libs/API/index.ts

  1. paginate - 6 parameters (line 228)

📂 src/libs/DistanceRequestUtils.ts

  1. getRateForDisplay - 7 parameters (line 148)
  2. getDistanceForDisplay - 6 parameters (line 181)
  3. getDistanceMerchant - 7 parameters (line 220)

📂 src/libs/IOUUtils.ts

  1. updateIOUOwnerAndTotal - 7 parameters (line 73)

📂 src/libs/KeyboardShortcut/index.ts

  1. subscribe - 10 parameters (line 132)

📂 src/libs/ModifiedExpenseMessage.ts

  1. buildMessageFragmentForValue - 8 parameters (line 48)

📂 src/libs/Navigation/helpers/createNormalizedConfigs.ts

  1. Arrow function - 6 parameters (line 81)

📂 src/libs/Notification/LocalNotification/BrowserNotifications.ts

  1. push - 7 parameters (line 49)

📂 src/libs/OptionsListUtils.ts

  1. getAttendeeOptions - 9 parameters (line 1653)
  2. getShareDestinationOptions - 6 parameters (line 1686)
  3. getMemberInviteOptions - 6 parameters (line 1741)
  4. formatSectionsFromSearchTerm - 7 parameters (line 1818)

📂 src/libs/Performance.tsx

  1. traceRender - 7 parameters (line 202)

📂 src/libs/SearchAutocompleteUtils.ts

  1. filterOutRangesWithCorrectValue - 7 parameters (line 138)
  2. parseForLiveMarkdown - 7 parameters (line 185)

📂 src/libs/SearchQueryUtils.ts

  1. buildFilterFormValuesFromQuery - 8 parameters (line 404)

📂 src/libs/SearchUIUtils.ts

  1. getSortedSections - 6 parameters (line 548)
  2. getOverflowMenu - 6 parameters (line 667)

📂 src/libs/SidebarUtils.ts

  1. getOrderedReportIDs - 8 parameters (line 190)

📂 src/libs/Violations/ViolationsUtils.ts

  1. getViolationsOnyxData - 7 parameters (line 166)

📂 src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx

  1. Arrow function - 19 parameters (line 177)

📂 src/pages/home/report/ContextMenu/ReportActionContextMenu.ts

  1. showContextMenu - 19 parameters (line 103)

📂 src/pages/settings/Wallet/InternationalDepositAccount/utils.ts

  1. getSubstepValues - 6 parameters (line 40)

📂 src/pages/settings/Wallet/WalletPage/WalletPage.tsx

  1. Arrow function - 7 parameters (line 148)

📂 src/pages/workspace/accounting/utils.tsx

  1. getAccountingIntegrationData - 9 parameters (line 42)

📂 src/pages/workspace/companyCards/utils.tsx

  1. getExportMenuItem - 6 parameters (line 20)

📂 src/pages/workspace/invoices/WorkspaceInvoiceVBASection.tsx

  1. Arrow function - 7 parameters (line 92)

📂 src/styles/utils/index.ts

  1. getZoomSizingStyle - 7 parameters (line 367)
  2. getIconWidthAndHeightStyle - 6 parameters (line 515)
  3. getButtonStyleWithIcon - 7 parameters (line 529)

📂 src/libs/fileDownload/index.ts

  1. Arrow function - 7 parameters (line 7)

📂 src/libs/fileDownload/index.android.ts

  1. Arrow function - 7 parameters (line 147)

📂 src/libs/fileDownload/index.desktop.ts

  1. Arrow function - 7 parameters (line 10)

📂 src/libs/fileDownload/index.ios.ts

  1. Arrow function - 7 parameters (line 107)

📂 src/libs/fileDownload/DownloadUtils.ts

  1. Arrow function - 7 parameters (line 33)

📂 src/components/EmojiPicker/EmojiPicker.tsx

  1. Arrow function - 7 parameters (line 81)

📂 src/components/ProfilingToolMenu/BaseProfilingToolMenu.tsx

  1. Curly braces are unnecessary here (line 173)

📂 src/components/SelectionList/BaseSelectionList.tsx

  1. Unexpected console statement (line 539)

📂 src/components/ShowContextMenuContext.ts

  1. showContextMenuForReport - 6 parameters (line 44)

📂 src/components/Tooltip/EducationalTooltip/measureTooltipCoordinate/index.android.ts

  1. Arrow function - 6 parameters (line 5)

📂 src/libs/GithubUtils.ts

  1. generateStagingDeployCashBodyAndAssignees - 9 parameters (line 274)

📂 src/ROUTES.ts

  1. getRoute - 7 parameters (line 352)
  2. getRoute - 6 parameters (line 510)
  3. getRoute - 6 parameters (line 525)
  4. getRoute - 6 parameters (line 565)
  5. getRoute - 7 parameters (line 661)
  6. getRoute - 6 parameters (line 666)
  7. getRoute - 6 parameters (line 671)
  8. getRoute - 7 parameters (line 710)
  9. getRoute - 6 parameters (line 718)

📂 tests/ui/LHNItemsPresence.tsx

  1. Arrow function - 6 parameters (line 91)
Functions with 10+ Parameters - Total: 9 functions
  1. getTrackExpenseInformation - 21 parameters (line 3079, src/libs/actions/IOU.ts)
  2. buildOnyxDataForInvoice - 19 parameters (line 1464, src/libs/actions/IOU.ts)
  3. buildOptimisticChatReport - 15 parameters (line 6015, src/libs/ReportUtils.ts)
  4. buildOptimisticMoneyRequestEntities - 15 parameters (line 6788, src/libs/ReportUtils.ts)
  5. createSplitsAndOnyxData - 16 parameters (line 5048, src/libs/actions/IOU.ts)
  6. buildOptimisticIOUReportAction - 13 parameters (line 5462, src/libs/ReportUtils.ts)
  7. buildOnyxDataForTrackExpense - 14 parameters (line 1862, src/libs/actions/IOU.ts)
  8. completeOnboarding - 10 parameters (line 4196, src/libs/actions/Report.ts)
  9. subscribe - 10 parameters (line 132, src/libs/KeyboardShortcut/index.ts)

Total: 9 functions

@melvin-bot melvin-bot bot added the Overdue label Mar 4, 2025
@neil-marcellini
Copy link
Contributor Author

I plan to work on this today

@melvin-bot melvin-bot bot removed the Overdue label Mar 5, 2025
@neil-marcellini
Copy link
Contributor Author

I wrote out a conversation prompt in a Slack canvas and I'll post it tomorrow morning in #expensify-open-source

@neil-marcellini
Copy link
Contributor Author

I posted the conversation in Slack here

@neil-marcellini
Copy link
Contributor Author

This PR is a really good example showing the default and also how it cleans up having to pass a bunch of parameters as undefined 🙂 refactor buildOptimisticChatReport to use a params object

@melvin-bot melvin-bot bot added the Overdue label Mar 24, 2025
@neil-marcellini
Copy link
Contributor Author

I'm setting a reminder to work on this on Wednesday.

@melvin-bot melvin-bot bot removed the Overdue label Mar 24, 2025
@neil-marcellini
Copy link
Contributor Author

Starting on this now

@neil-marcellini neil-marcellini mentioned this issue Mar 26, 2025
Merged
50 tasks
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Mar 26, 2025
@neil-marcellini
Copy link
Contributor Author

Hi @Krishna2323 would you please implement your proposal to add the ES Lint rule? As we discussed in Slack here please make it only error at 10+ params and add it to the changed files es lint check only for now.

@neil-marcellini
Copy link
Contributor Author

If you're busy with higher priorities please let me know and I can open it up for others.

@Krishna2323
Copy link
Contributor

@neil-marcellini, thanks for the assignment. I will open up a PR today.

@Krishna2323 Krishna2323 mentioned this issue Mar 28, 2025
Merged
49 tasks
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 28, 2025
@Krishna2323
Copy link
Contributor

@neil-marcellini, the PR is ready for review. Do we need someone else to review it? BTW, I added the rule in .eslintrc.js because currently, there are no functions with 10 or more parameters.

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Apr 7, 2025
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Apr 7, 2025
@melvin-bot melvin-bot bot changed the title Update style guide to require parameter objects for functions with many parameters [Due for payment 2025-04-14] Update style guide to require parameter objects for functions with many parameters Apr 7, 2025
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Apr 7, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 7, 2025
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Apr 7, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.23-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-04-14. 🎊

For reference, here are some details about the assignees on this issue:

  • @Krishna2323 requires payment (Needs manual offer from BZ)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 14, 2025
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Apr 14, 2025

Issue is ready for payment but no BZ is assigned. @slafortune you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Apr 14, 2025

Payment Summary

Upwork Job

BugZero Checklist (@slafortune)

  • I have verified the correct assignees and roles are listed above and updated the necessary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@slafortune slafortune removed their assignment Apr 14, 2025
@slafortune slafortune added the Bug Something is broken. Auto assigns a BugZero manager. label Apr 14, 2025
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Apr 14, 2025

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@slafortune slafortune self-assigned this Apr 14, 2025
@slafortune slafortune removed the Bug Something is broken. Auto assigns a BugZero manager. label Apr 14, 2025
@slafortune
Copy link
Contributor

Unexpected OOO

@sonialiap
Copy link
Contributor

@neil-marcellini to confirm, I see that most of the sub-issues are priced at $125, is that the expected price here?

@neil-marcellini
Copy link
Contributor Author

Maybe $25? It was pretty easy

@sonialiap
Copy link
Contributor

Payment summary:

@Krishna2323
Copy link
Contributor

@neil-marcellini, it was a relatively small task, but I did spend a bit of time running the rule, checking violations, and organizing the message + stats. Would $50 work for you?

If not, I’m happy to settle at $25 this time since it was light — just wanted to highlight that it’s more than just a one-liner config change 🙂

@melvin-bot melvin-bot bot added the Overdue label Apr 21, 2025
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Apr 21, 2025

@slafortune, @sonialiap, @neil-marcellini, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@neil-marcellini
Copy link
Contributor Author

Sure I understand, but let's stick with $25. All PRs require some time but this was about as small as they get.

@melvin-bot melvin-bot bot removed the Overdue label Apr 21, 2025
@sonialiap
Copy link
Contributor

Payment made

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

@slafortune slafortune

@sonialiap sonialiap

@neil-marcellini neil-marcellini

@Krishna2323 Krishna2323

Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Improvement Item broken or needs improvement.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@slafortune @sonialiap @neil-marcellini @abdulrahuman5196 @Krishna2323