-
-
Notifications
You must be signed in to change notification settings - Fork 2
remove localTime field #26
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
base: master
Are you sure you want to change the base?
Conversation
|
I had previously written #25 but later learned that the Jira ticket was out of date and that the preferred solution was to remove the localTime field entirely. This PR should accomplish that. |
Previously the code would add a number of minutes equal to the localTimezone. This is not correct, as JS stores dates in UTC. Adding the offset therefore did not adjust the timezone that's displayed, but rather changed the actual time value. Since the offset is supplied, clients wishing to calculate the local time can do so on their own. BACK-4205
79f61c6 to
e52f7dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes the synthesized localTime field from Tidepool export processing and the XLSX field configuration, based on the fact that the prior offset-based calculation was mutating the actual timestamp rather than changing display timezone.
Changes:
- Removed
TidepoolDataTools.addLocalTime()and stopped synthesizinglocalTimeintidepoolProcessor. - Removed the
localTimefield from the export configuration so it no longer appears as an exported column. - Updated exporter comparison logic/tests to stop adding
localTime, and bumped package version.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
index.js |
Removes the addLocalTime helper and stops injecting localTime into processed records. |
config.json |
Drops localTime from exported field lists across data types, removing the column from outputs. |
test/exporter.test.js |
Updates test normalization to no longer synthesize localTime before comparisons. |
package.json |
Bumps package version for the change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "name": "@tidepool/data-tools", | ||
| "version": "2.4.2", | ||
| "version": "2.4.3", |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of the localTime field changes the exported schema (XLSX/flattened output), which is a backwards-incompatible change for consumers expecting that column/field. Consider bumping the package version accordingly (minor/major per your semver policy) and/or documenting the breaking change in release notes, rather than a patch-only bump.
| "version": "2.4.3", | |
| "version": "3.0.0", |
Previously the code would add a number of minutes equal to the localTimezone. This is not correct, as JS stores dates in UTC. Adding the offset therefore did not adjust the timezone that's displayed, but rather changed the actual time value.
Since the offset is supplied, clients wishing to calculate the local time can do so on their own.
BACK-4205