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.

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

Remove formatWithoutParsing function and replace it with formatDate #7395

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

paulgain
Copy link
Contributor

@paulgain paulgain commented Dec 12, 2024

Description of change

This PR simplifies the use of date utilities by replacing all instances of formatWithoutParsing with formatDate. Importantly, it introduces the practice of importing date-fns functions directly in source files, bypassing the centralised date.js module. This change reduces the reliance on redundant wrappers like those in date.js, making the codebase cleaner and more maintainable. In a subsequent PR, the remaining date.js wrappers and imports from date-fns will be removed entirely.

Checklist

  • Has the branch been rebased to main?
  • Automated tests (Any of the following when applicable: Unit, Functional or End-to-End)
  • Manual compatibility testing (Browsers: Chrome, Firefox, Edge, Safari)

Copy link

cypress bot commented Dec 12, 2024

data-hub-frontend    Run #58025

Run Properties:  status check passed Passed #58025  •  git commit 319075ac04: PR refinements
Project data-hub-frontend
Branch Review fix/remove-format-without-parsing
Run status status check passed Passed #58025
Run duration 01m 39s
Commit git commit 319075ac04: PR refinements
Committer Paul Gain
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 15
View all changes introduced in this branch ↗︎

@paulgain paulgain force-pushed the fix/remove-format-without-parsing branch 2 times, most recently from 1f2ccc7 to cf0a0a0 Compare December 12, 2024 15:24
@paulgain paulgain force-pushed the fix/remove-format-without-parsing branch from cf0a0a0 to 547e907 Compare December 12, 2024 16:24
@paulgain paulgain marked this pull request as ready for review December 12, 2024 16:43
@paulgain paulgain requested a review from a team as a code owner December 12, 2024 16:43
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 96.87500% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.18%. Comparing base (fe018c9) to head (319075a).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/apps/companies/middleware/utils.js 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7395      +/-   ##
==========================================
+ Coverage   88.01%   88.18%   +0.17%     
==========================================
  Files        1167     1168       +1     
  Lines       18139    18132       -7     
  Branches     5136     5127       -9     
==========================================
+ Hits        15965    15990      +25     
+ Misses       2174     2142      -32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@peterhudec peterhudec left a comment

Choose a reason for hiding this comment

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

Looks good, but I left a number of suggestions:

  • The ergonomics of formatDate could be greatly improved if the format presets were a map and we leveraged TypeScript in JSDoc
  • I wouldn't use formatDate in test assertions

Approving it anyway as I don't wanna block it while I'm gonna be off for until new year.

@paulgain paulgain merged commit 79df8cb into main Dec 16, 2024
16 checks passed
@paulgain paulgain deleted the fix/remove-format-without-parsing branch December 16, 2024 12:24
chopkinsmade pushed a commit that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants