Fix/winal-radix-correction by drewsonne · Pull Request #94 · drewsonne/maya-dates

drewsonne · 2026-01-02T16:37:29Z

No description provided.

src/__tests__/full-date.spec.ts

Copilot

Pull request overview

This pull request addresses a winal-radix correction by adding comprehensive test infrastructure based on historical Maya date correlations from Mesoweb's "Table of Palenque Rulers". The changes introduce a substantial JSON dataset containing 3,647 correlation entries and testing utilities to validate the library's Maya calendar calculations against historically documented dates.

Key Changes

Added JSON dataset with historical Maya date correlations (3,647 entries across 4 correlation constants)
Implemented test data loader utility with filtering and query helpers
Created comprehensive test suites validating correlations, calendar rounds, and full dates

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`src/__tests__/test-data-loader.ts`	Utility module for loading and querying the JSON correlation dataset
`src/__tests__/maya_date_correlations.json`	Historical correlation data from Mesoweb source with 3,647 entries
`src/__tests__/lc/western/western.spec.ts`	Added tests validating Long Count to Western date conversions using JSON data
`src/__tests__/full-date.spec.ts`	Added historical full date validation tests with orthographic variation handling
`src/__tests__/correlation-validation.spec.ts`	Comprehensive test suite for correlation constants, calendar rounds, and data quality
`.mocharc.json`	Mocha test runner configuration for TypeScript support

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-02T16:39:48Z

src/__tests__/full-date.spec.ts

+    // of Maya day/month names), some entries may not parse. This is expected.
+    // The JSON data uses various transliteration systems that may differ from
+    // the library's expected spellings.
+    console.log(`Calendar Round parsing: ${successCount} succeeded, ${failureCount} failed (orthographic variations expected)`);


Using console.log in test code can clutter test output and is generally discouraged. Consider removing this or using a proper test reporting mechanism if you need to track these statistics.

Suggested change

console.log(`Calendar Round parsing: ${successCount} succeeded, ${failureCount} failed (orthographic variations expected)`);

src/__tests__/lc/western/western.spec.ts

Copilot · 2026-01-02T16:39:48Z

src/__tests__/lc/western/western.spec.ts

+          const year = correlation.western_date.split('-')[0];
+          const gregorianDate = `${lc.gregorian}`;
+          // Remove leading zeros for comparison (e.g., 0397 -> 397)
+          expect(gregorianDate).to.include(year.replace(/^0+/, ''));


The year comparison logic using replace(/^0+/, '') and string inclusion is fragile and may produce false positives. A date with year "397" would match "1397" or "2397". Consider parsing the dates into comparable formats or extracting the year component more precisely for comparison.

@copilot I think a better solution is just to match on a single format (eg, iso8601)

Copilot · 2026-01-02T16:39:48Z

src/__tests__/lc/western/western.spec.ts

+        const expectedYear = correlation.western_date.split('-')[0];
+        const gregorianDate = `${lc.gregorian}`;
+        // Remove leading zeros for comparison
+        expect(gregorianDate).to.include(expectedYear.replace(/^0+/, ''));


The year comparison logic using replace(/^0+/, '') and string inclusion is fragile and may produce false positives. A date with year "397" would match "1397" or "2397". Consider parsing the dates into comparable formats or extracting the year component more precisely for comparison.

Copilot · 2026-01-02T16:39:49Z

src/__tests__/correlation-validation.spec.ts

+            // This is a placeholder - you may need to adjust date format comparison
+            expect(actualDate).to.include(correlation.western_date.split('-')[0]); // At least check year


This test is defined inside a forEach loop but then creates a nested it block, which is incorrect test structure. The it block should be the outer loop iterator, not defined inside the forEach. This will cause only the last iteration to be tested or may fail to register tests properly.

@copilot open a new pull request to apply changes based on this feedback

src/__tests__/full-date.spec.ts

Copilot · 2026-01-02T16:39:49Z

src/__tests__/test-data-loader.ts

+let correlationData: CorrelationTestData | null = null;
+
+/**
+ * Loads the Maya date correlation test data from JSON file
+ */
+export function loadCorrelationData(): CorrelationTestData {
+  if (!correlationData) {
+    const jsonPath = path.join(__dirname, 'maya_date_correlations.json');
+    const rawData = fs.readFileSync(jsonPath, 'utf8');
+    correlationData = JSON.parse(rawData) as CorrelationTestData;
+  }


The module-level mutable state correlationData is used for caching. While this works, consider using a more explicit caching pattern or documenting this behavior, as module-level mutable state can make testing and debugging more difficult.

Suggested change

let correlationData: CorrelationTestData | null = null;

/**

* Loads the Maya date correlation test data from JSON file

*/

export function loadCorrelationData(): CorrelationTestData {

if (!correlationData) {

const jsonPath = path.join(__dirname, 'maya_date_correlations.json');

const rawData = fs.readFileSync(jsonPath, 'utf8');

correlationData = JSON.parse(rawData) as CorrelationTestData;

}

const jsonPath = path.join(__dirname, 'maya_date_correlations.json');

const rawData = fs.readFileSync(jsonPath, 'utf8');

const correlationData: CorrelationTestData = JSON.parse(rawData) as CorrelationTestData;

/**

* Loads the Maya date correlation test data from JSON file

*/

export function loadCorrelationData(): CorrelationTestData {

Copilot

Pull request overview

Copilot reviewed 5 out of 7 changed files in this pull request and generated 16 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-02T17:11:55Z

src/__tests__/test-data-loader.ts

+ */
+export function loadCorrelationData(): CorrelationTestData {
+  if (!correlationData) {
+    const jsonPath = path.join(__dirname, 'maya_date_correlations.json');


The path references 'maya_date_correlations.json' but the actual file in the diff is 'maya_date_correlations.json.bak'. This path mismatch will cause a file not found error at runtime. Either the filename needs to be updated to match the path, or the path needs to include the '.bak' extension.

Suggested change

const jsonPath = path.join(__dirname, 'maya_date_correlations.json');

const jsonPath = path.join(__dirname, 'maya_date_correlations.json.bak');

Copilot · 2026-01-02T17:11:55Z

src/__tests__/correlation-validation.spec.ts

+import LongCountFactory from '../factory/long-count';
+import FullDateFactory from '../factory/full-date';
+import {getCorrelationConstant} from '../lc/correlation-constant';
+


The comment references a "winal radix correction" in the PR title, but there's no documentation explaining what this correction is or why these tests validate it. Consider adding a comment block at the top of this test file explaining the purpose of these tests and how they relate to the winal radix correction mentioned in the PR title.

Suggested change

/**

* These tests validate Maya Long Count ↔ western date correlations using the JSON

* fixture data and the conversion factories. In particular, they exercise logic

* affected by the "winal radix correction" mentioned in the PR title.

*

* The winal radix correction refers to how 20‑day winals are handled when mapping

* Long Count positions to a fixed day count (JDN) and then to Gregorian/Julian

* calendar dates. Historically, small off‑by‑one errors in this radix handling

* can shift whole correlation constants by one or more days.

*

* By:

* - loading the canonical correlation dataset,

* - validating the GMT correlation constant (584285) against direct historical

* source entries, and

* - comparing western dates produced under neighboring correlation constants

* (e.g. 584283–584286),

* these tests ensure that the current implementation of the winal radix, and the

* resulting correlation constants, produce stable and internally consistent dates.

*/

Copilot · 2026-01-02T17:11:55Z

src/__tests__/maya_date_correlations.json.bak

@@ -0,0 +1,3647 @@
+{
+  "metadata": {
+    "generated_at": "2026-01-02",


The file has a ".bak" extension which suggests this is a backup file. Backup files should typically not be committed to version control. If this is intended to be the actual test data file, it should be renamed to remove the ".bak" extension. If it's truly a backup, it should be excluded from the repository.

Suggested change

"generated_at": "2026-01-02",

"generated_at": "2025-01-02",

Copilot · 2026-01-02T17:11:56Z

src/__tests__/correlation-validation.spec.ts

+  getAvailableCorrelations,
+  CorrelationData


'CorrelationData' is defined but never used.

Suggested change

getAvailableCorrelations,

CorrelationData

getAvailableCorrelations

Copilot · 2026-01-02T17:11:56Z

src/__tests__/correlation-validation.spec.ts

+          // Test that Calendar Round actually parses - this will fail if spellings don't match
+          const fullDateString = `${data.calendar_round} ${data.maya_long_count}`;
+          const fullDate = factory.parse(fullDateString);
+          expect(fullDate).to.not.be.null;


Expected an assignment or function call and instead saw an expression.

Suggested change

expect(fullDate).to.not.be.null;

expect(fullDate).to.not.equal(null);

Copilot · 2026-01-02T17:11:58Z

src/__tests__/full-date.spec.ts

@@ -67,3 +72,49 @@ it('isPartial should detect wildcards', () => {
  expect(fd3.isPartial()).to.be.true;
  expect(fd4.isPartial()).to.be.true;


Expected an assignment or function call and instead saw an expression.

Copilot · 2026-01-02T17:11:58Z

src/__tests__/full-date.spec.ts

+      // Parse should succeed - if it fails, the spellings don't match
+      const fullDate = new FullDateFactory().parse(fullDateString);
+
+      expect(fullDate).to.not.be.null;


Expected an assignment or function call and instead saw an expression.

Suggested change

expect(fullDate).to.not.be.null;

expect(fullDate).to.not.equal(null);

Copilot · 2026-01-02T17:11:58Z

src/__tests__/full-date.spec.ts

+      const lcString = correlation.maya_long_count;
+      const lc = new LongCountFactory().parse(lcString);
+
+      expect(lc).to.not.be.null;


Expected an assignment or function call and instead saw an expression.

Copilot · 2026-01-02T17:11:58Z

src/__tests__/lc/western/western.spec.ts

+import {
+  getGMTCorrelationData,
+  getDirectSourceData,
+  findCorrelation,


'findCorrelation' is defined but never used.

Suggested change

findCorrelation,

Copilot · 2026-01-02T17:11:58Z

src/__tests__/lc/western/western.spec.ts

+        const lc = lcFactory.parse(correlation.maya_long_count).setCorrelationConstant(corr);
+
+        // Validate the Long Count parses correctly
+        expect(lc).to.not.be.null;


Expected an assignment or function call and instead saw an expression.

Suggested change

expect(lc).to.not.be.null;

expect(lc).to.not.equal(null);

drewsonne · 2026-01-02T17:12:20Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-01-02T17:12:29Z

@drewsonne I've opened a new pull request, #95, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-01-02T17:28:38Z

@drewsonne I've opened a new pull request, #100, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-01-02T17:29:10Z

@drewsonne I've opened a new pull request, #101, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-01-02T17:29:29Z

@drewsonne I've opened a new pull request, #102, to work on those changes. Once the pull request is ready, I'll request review from you.

…ntation Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Copilot · 2026-01-02T17:50:02Z

src/__tests__/test-data-loader.ts

+/**
+ * Loads the Maya date correlation test data from JSON file
+ */
+export function loadCorrelationData(): CorrelationTestData {


Missing explicit return type annotation. According to the coding guidelines, all public functions must have explicit return types. Add : CorrelationData[] as the return type.

Copilot · 2026-01-02T17:50:04Z

src/__tests__/full-date.spec.ts

+  expect(fd2.isPartial()).to.equal(true);
+  expect(fd3.isPartial()).to.equal(true);
+  expect(fd4.isPartial()).to.equal(true);


Inconsistent assertion style. Lines 70-72 use .to.equal(true) while the rest of the codebase (including line 69) uses .to.be.true. For consistency with Chai best practices and the existing code, use .to.be.true instead of .to.equal(true).

Suggested change

expect(fd2.isPartial()).to.equal(true);

expect(fd3.isPartial()).to.equal(true);

expect(fd4.isPartial()).to.equal(true);

expect(fd2.isPartial()).to.be.true;

expect(fd3.isPartial()).to.be.true;

expect(fd4.isPartial()).to.be.true;

Copilot · 2026-01-02T17:50:05Z

src/__tests__/full-date.spec.ts

@@ -63,7 +67,53 @@ it('isPartial should detect wildcards', () => {
  const fd4 = new FullDateFactory().parse('4 Ajaw * Kumk\'u 9.17.0.0.*');

  expect(fd1.isPartial()).to.be.false;


Expected an assignment or function call and instead saw an expression.

Suggested change

expect(fd1.isPartial()).to.be.false;

expect(fd1.isPartial()).to.equal(false);

…age control Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

- Fixed BCE year conversion to use astronomical year numbering (1 BCE = year 0) - Corrected verification logic in GregorianFactory.parse() for BCE dates - Removed asterisk suffix from toString() for threshold dates (Oct 4/15, 1582) - Cleaned up test data to remove asterisk markers from date strings - Simplified Gregorian offset table to remove incorrect intermediate ranges - All 466 tests now passing, including 5 previously failing edge cases

- Renamed correlation-validation.spec.ts.skip back to .spec.ts - All comprehensive data validation tests now pass - Tests validate JSON dataset structure, metadata, helper functions - Adds 20 additional passing tests for data quality and structure

This temporary debugging file was used to investigate year boundary failures that have now been fixed. No longer needed with all tests passing.

This diagnostic test was used to investigate year boundary failures that have now been resolved. The actual functionality is covered by western.spec.ts and correlation-validation.spec.ts.

This file was used to debug offset boundary calculations but only prints diagnostic output without actual assertions. The functionality is fully tested in western.spec.ts with proper assertions.

The variable held all Gregorian calendar dates from the dataset, not a sample. Also updated the test suite description for clarity.

src/__tests__/lc/western/western.spec.ts

+      const g = gregorianFactory.parse(dc.gregorian);
+      // Verify that the parsed date matches the expected Gregorian date string
+      // The toString() method should return the same format as the input (without asterisk if not threshold)
+      const expectedDate = dc.gregorian.replace('*', '').trim();


In general, to fix this kind of problem, ensure that any sanitization or marker-removal that should apply to all occurrences uses a global replacement (e.g., a regex with the g flag) or a built-in method that replaces all occurrences (such as replaceAll).

In this specific case, we want to remove all * characters from dc.gregorian before comparing it to the parsed date string. The minimal, behavior-preserving fix is to change line 82 from using string.replace('*', '') to a global replacement. Since this is TypeScript targeting typical Node/test environments, using a regular expression with the global flag is reliable and avoids any dependency changes. Thus we should update line 82 to:

const expectedDate = dc.gregorian.replace(/\*/g, '').trim();

This ensures that any and all * characters in the string are removed, not just the first one, while keeping the rest of the test logic unchanged. No new imports or helper functions are needed; the change is confined to the single line in src/__tests__/lc/western/western.spec.ts.

Copilot AI review requested due to automatic review settings January 2, 2026 16:37

drewsonne force-pushed the fix/winal-radix-correction branch from 3efa6ab to 33038b6 Compare January 2, 2026 16:37

Copilot started reviewing on behalf of drewsonne January 2, 2026 16:37 View session

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

src/__tests__/full-date.spec.ts Fixed Show fixed Hide fixed

src/__tests__/full-date.spec.ts Fixed Show fixed Hide fixed

Copilot AI reviewed Jan 2, 2026

View reviewed changes

drewsonne force-pushed the fix/winal-radix-correction branch from 33038b6 to e569144 Compare January 2, 2026 17:00

feat: add date checks

003c956

drewsonne force-pushed the fix/winal-radix-correction branch from e569144 to 003c956 Compare January 2, 2026 17:06

drewsonne requested a review from Copilot January 2, 2026 17:08

Copilot started reviewing on behalf of drewsonne January 2, 2026 17:08 View session

Copilot AI reviewed Jan 2, 2026

View reviewed changes

Copilot AI mentioned this pull request Jan 2, 2026

Address code review feedback: remove backup file, fix lint errors, add documentation #95

Merged

This was referenced Jan 2, 2026

Load entire dataset for correlation validation tests #100

Merged

Implement ISO 8601 date support throughout library #101

Open

Copilot AI mentioned this pull request Jan 2, 2026

Fix test structure: move forEach outside it block for proper test isolation #102

Merged

Copilot AI and others added 3 commits January 2, 2026 17:31

Fix code review issues: remove .bak file, fix lint errors, add docume…

82a9465

…ntation Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Improve import organization: move type import to top

017f301

Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Fix test structure: move forEach outside it block

41febbf

Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Copilot AI review requested due to automatic review settings January 2, 2026 17:47

Copilot started reviewing on behalf of drewsonne January 2, 2026 17:48 View session

Copilot AI reviewed Jan 2, 2026

View reviewed changes

Copilot AI and others added 4 commits January 2, 2026 20:43

Add configurable TEST_SAMPLE_SIZE environment variable for test cover…

905c85c

…age control Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Add input validation for TEST_SAMPLE_SIZE environment variable

19f7be3

Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Improve code quality: Add JSDoc and extract helper function

62dc667

Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

Remove filtering logic to load entire dataset

2879f29

Co-authored-by: drewsonne <233054+drewsonne@users.noreply.github.com>

drewsonne added 7 commits January 2, 2026 20:43

feat: fix threshold errors

e0cd9a3

Remove obsolete diagnostic-test.ts file

51dd5e7

This temporary debugging file was used to investigate year boundary failures that have now been fixed. No longer needed with all tests passing.

Remove obsolete diagnostic-boundary.spec.ts

e2e48db

This diagnostic test was used to investigate year boundary failures that have now been resolved. The actual functionality is covered by western.spec.ts and correlation-validation.spec.ts.

Remove obsolete calculate-offsets.spec.ts diagnostic file

3e60620

This file was used to debug offset boundary calculations but only prints diagnostic output without actual assertions. The functionality is fully tested in western.spec.ts with proper assertions.

Rename misleading 'sampleData' variable to 'gregorianData'

97f1cfe

The variable held all Gregorian calendar dates from the dataset, not a sample. Also updated the test suite description for clarity.

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

drewsonne closed this Jan 2, 2026

		// This is a placeholder - you may need to adjust date format comparison
		expect(actualDate).to.include(correlation.western_date.split('-')[0]); // At least check year

	const jsonPath = path.join(__dirname, 'maya_date_correlations.json');
	const jsonPath = path.join(__dirname, 'maya_date_correlations.json.bak');

+/**
+ * These tests validate Maya Long Count ↔ western date correlations using the JSON
+ * fixture data and the conversion factories. In particular, they exercise logic
+ * affected by the "winal radix correction" mentioned in the PR title.
+ *
+ * The winal radix correction refers to how 20‑day winals are handled when mapping
+ * Long Count positions to a fixed day count (JDN) and then to Gregorian/Julian
+ * calendar dates. Historically, small off‑by‑one errors in this radix handling
+ * can shift whole correlation constants by one or more days.
+ *
+ * By:
+ *  - loading the canonical correlation dataset,
+ *  - validating the GMT correlation constant (584285) against direct historical
+ *    source entries, and
+ *  - comparing western dates produced under neighboring correlation constants
+ *    (e.g. 584283–584286),
+ * these tests ensure that the current implementation of the winal radix, and the
+ * resulting correlation constants, produce stable and internally consistent dates.
+ */

	getAvailableCorrelations,
	CorrelationData
	getAvailableCorrelations

	expect(fullDate).to.not.be.null;
	expect(fullDate).to.not.equal(null);

		@@ -67,3 +72,49 @@ it('isPartial should detect wildcards', () => {
		expect(fd3.isPartial()).to.be.true;
		expect(fd4.isPartial()).to.be.true;

		@@ -63,7 +67,53 @@ it('isPartial should detect wildcards', () => {
		const fd4 = new FullDateFactory().parse('4 Ajaw * Kumk\'u 9.17.0.0.*');

		expect(fd1.isPartial()).to.be.false;

	expect(fd1.isPartial()).to.be.false;
	expect(fd1.isPartial()).to.equal(false);

Comments

Conversation

drewsonne commented Jan 2, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

drewsonne Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

drewsonne Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

drewsonne commented Jan 2, 2026

Uh oh!

Copilot AI commented Jan 2, 2026

Uh oh!

Copilot AI commented Jan 2, 2026

Uh oh!

Copilot AI commented Jan 2, 2026

Uh oh!

Copilot AI commented Jan 2, 2026

Uh oh!

Copilot AI left a comment