Skip to content

Comments

Test improvements to calcDateOffsetStr and other new tests and edge case fix#698

Merged
jgclark merged 4 commits intomainfrom
jgclark/issue654
Nov 7, 2025
Merged

Test improvements to calcDateOffsetStr and other new tests and edge case fix#698
jgclark merged 4 commits intomainfrom
jgclark/issue654

Conversation

@jgclark
Copy link
Collaborator

@jgclark jgclark commented Nov 5, 2025

No description provided.

Copy link
Collaborator

@dwertheimer dwertheimer left a comment

Choose a reason for hiding this comment

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

@jgclark I was looking through this PR code and I find it confusing on why/when it was using ISO weeks in some cases when NP weeks would be appropriate. I asked Cursor and it agreed, writing:

There's a significant issue in the logic at lines 1327-1329
The issue: The variable wasISOWeekInput checks if the input matches RE_NP_WEEK_SPEC (the pattern YYYY-Wnn), but ISO weeks and NotePlan weeks use the same format - there's no way to visually distinguish them!
This means:
⚠️ When the input is in YYYY-Wnn format, the code always assumes it's ISO and uses formatISOWeek (Monday-start)
❌ It will never use formatNPWeek (which respects user preferences) when the base input is a week string

@EduardMe does not use ISO weeks. So I don't think the plugins should either. I am going to suggest a refactor that will remove the ISO weeks and force NP Weeks (and will provide the tests and mocks you asked for). Of course you can reject if you disagree.

@dwertheimer
Copy link
Collaborator

Screen Cap 2025-11-05 at 17 12 28@2x

@dwertheimer
Copy link
Collaborator

Summary of calcOffsetDateStr Refactoring: NotePlan Week Support

Change Made

Removed all ISO week logic from calcOffsetDateStr() to always use NotePlan weeks.
Previously, the function tried to distinguish between ISO and NotePlan week formats
(impossible since both use YYYY-Wnn), causing unpredictable behavior. Now it
consistently calls formatNPWeek() which respects the user's NotePlan week start
preference (Sunday vs Monday) via the Calendar API.

What Is Enforced

  • Single source of truth: All week formatting uses NotePlan's Calendar API
  • User preference respected: Week start day (Sunday/Monday/etc) comes from NotePlan settings
  • No ambiguity: Removed the impossible-to-implement "detect ISO vs NP week" logic
  • Consistency: Same YYYY-Wnn input always produces same output for a given Calendar configuration

26 New Tests Cover These Edges

Week Start Day Differences (12 tests)

  • Sunday-start weeks (US style): 6 tests validating week calculations when weeks start on Sunday
  • Monday-start weeks (ISO/European style): 6 tests validating week calculations when weeks start on Monday
  • Ensures both week numbering systems work correctly with user preferences

Year Boundary Crossings (8 tests)

  • Forward transitions: W52/W53 → W01 of next year
  • Backward transitions: W01 → W52/W53 of previous year
  • Tests both Sunday and Monday start configurations
  • Validates year number changes correctly in week strings

Week 53 Edge Cases (4 tests)

  • Years that have 53 weeks (e.g., 2020, 2015)
  • Transitioning from W53 to W01 and back
  • Ensures rare 53-week years don't break calculations

adaptOutputInterval Format Control (6 tests)

  • 'base': Preserves input format (week → week, day → day)
  • 'offset': Uses offset unit's format (day+weeks → week output)
  • 'week'/'day': Forces specific output format
  • 'longer'/'shorter': Adapts format based on granularity hierarchy
  • Validates format conversion logic respects NotePlan week conventions

Multiple Week Offsets (4 tests)

  • Large offsets (10, 20, 26 weeks) crossing year boundaries
  • Validates cumulative week math doesn't drift or break at year transitions

All tests use exact assertions (toEqual('2024-W52')) instead of loose regex patterns,
ensuring they catch actual bugs rather than passing on any week-like string.

@dwertheimer
Copy link
Collaborator

@jgclark on a related note, this PR is failing because of:

Build of plugin: "/Users/runner/work/plugins/plugins/jgclark.WindowTools" failed Error [RollupError]: jgclark.WindowTools/src/WTHelpers.js (14:9): "MAIN_SIDEBAR_CONTROL_BUILD_VERSION" is not exported by "helpers/NPWindows.js", imported by "jgclark.WindowTools/src/WTHelpers.js".

@jgclark
Copy link
Collaborator Author

jgclark commented Nov 7, 2025

Thanks, David, for looking at this carefully and providing new mocks and tests, which is all most helpful.

It's odd to see the way Cursor is so happily dissing its earlier work that it did for me. But if there's now a way to only use NP weeks, not a mix of ISO and NP ones, then that's indeed a good step forward.

Much earlier on I was clearly trying to avoid use of NotePlan.Calendar, as it wouldn't be available from HTMLWindows, but having a look now I think we don't in practice have examples of it. I'll have a look to see whether these functions now need to live in NPDateTime.js not dateTIme.js.

@jgclark
Copy link
Collaborator Author

jgclark commented Nov 7, 2025

@jgclark on a related note, this PR is failing because of:

Build of plugin: "/Users/runner/work/plugins/plugins/jgclark.WindowTools" failed Error [RollupError]: jgclark.WindowTools/src/WTHelpers.js (14:9): "MAIN_SIDEBAR_CONTROL_BUILD_VERSION" is not exported by "helpers/NPWindows.js", imported by "jgclark.WindowTools/src/WTHelpers.js".

That's odd, as WindowTools on main was and is building for me OK. But I've fixed the issue.

@jgclark jgclark merged commit c979c49 into main Nov 7, 2025
3 checks passed
@jgclark jgclark deleted the jgclark/issue654 branch November 7, 2025 12:29
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