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

truncate timezone ID only if it is too long #311

Merged
merged 2 commits into from
Feb 9, 2025

Conversation

jhoule86
Copy link
Contributor

@jhoule86 jhoule86 commented Feb 9, 2025

This is an attempt at improving the efficiency and readability of the recently introduced Time Zone truncation code and its accompanying logs.

The current code in master will always be calling timezone.id.take(MAX_TIMEZONE_NAME_LENGTH) twice, for all Time Zones:

  1. To log the expected output before populating the packet to be sent to the watch
  2. When actually populating the packet

The proposed code enforces consistency between the log and the packet sent to the watch, and only does costly String operations when absolutely necessary, by checking the length against the maximum.

)

Logging.i("Sending current time to watch: $now, timezone: ${normalizedZone}, offset: $timezoneOffsetMinutes")
Copy link
Contributor Author

@jhoule86 jhoule86 Feb 9, 2025

Choose a reason for hiding this comment

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

Moving the log down to this line is admittedly trivial, but I felt that it helped the flow of the code, and prepares for the potential change to
Logging.i("Sending current time to watch: ${updateTimePacket}")
if/when there is a good toString implemented.

val normalizedZone = timezone.id
if (normalizedZone.length > MAX_TIMEZONE_NAME_LENGTH) {
normalizedZone = timezone.id.take(MAX_TIMEZONE_NAME_LENGTH)
Logging.i("Time Zone ${timezone.id} exceeds maximum value length and has been truncated to ${normalizedZone}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this truncation

  1. is a recent addition - a436039
  2. most likely is an edge case

I felt it was worth being noisy about it in this separate log, instead of trying to shoehorn it into the existing log.

@crc-32 crc-32 merged commit 5ed8afa into pebble-dev:master Feb 9, 2025
1 check failed
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.

2 participants