Skip to content

Conversation

@qiichos
Copy link
Contributor

@qiichos qiichos commented Apr 23, 2025

Summary

This pull request adds a new option, useCurrentYearIfMissing, to the rule.
When enabled, this option automatically supplements the current year for date strings that do not explicitly include a year (e.g., 4月23日(水) or 4/23(Wed)), allowing weekday validation to work as expected.

Details

  • Added the useCurrentYearIfMissing option (default: false)
  • When enabled, the rule re-parses date strings without a year, injecting the current year only for validation
  • Added tests to cover this new behavior
    • added tests use sinon to mock the current date, ensuring stable results regardless of the actual year
  • Updated the README to document the new option
  • Dropped support and tests for formats like 4-23(金) because chrono-node parses them as times, not dates.

Example

When useCurrentYearIfMissing is enabled in 2025,

🟢 4月23日(水)
🟢 4/23(Wed)
❌ 4月23日(月) → 4月23日(水)
❌ 4/23(Mon) → 4/23(Wed)

Motivation

In many Japanese and multilingual documents, dates are often written without the year.
This option makes the rule more practical for such use cases, while keeping the default behavior unchanged for backward compatibility.

Notes

The new logic only affects cases where the year is missing in the original text and the option is enabled.
🦺 Existing behavior and options are not affected.

Thanks in advance!

@azu azu requested a review from Copilot April 23, 2025 09:08
Copy link

Copilot AI left a 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 pull request introduces the new option useCurrentYearIfMissing, allowing the rule to supplement the current year for date strings missing a year so that weekday validation works correctly. Key changes include:

  • Adding new logic in the rule to re-parse date strings with the current year when needed.
  • Updating tests to cover both valid and invalid scenarios using the new option.
  • Enhancing documentation in README.md to describe the useCurrentYearIfMissing option.

Reviewed Changes

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

File Description
test/textlint-rule-date-weekday-mismatch-test.js Added tests to verify behavior when the useCurrentYearIfMissing option is enabled
src/textlint-rule-date-weekday-mismatch.js Introduced addYearToDateText and updated reporter logic for re-parsing dates
README.md Documented the new useCurrentYearIfMissing option

@qiichos
Copy link
Contributor Author

qiichos commented Apr 23, 2025

I realized that these tests will fail next year because the expected results depend on the current year...
I am currently working on a fix by injecting a mock for the current date using sinon.

@qiichos
Copy link
Contributor Author

qiichos commented Apr 23, 2025

@azu
Thanks for reviewing this PR!
I just updated the test code to ensure that they always produce the same results regardless of when they are run, I used sinon to mock the current date and fixed it to 2025-04-23 during testing.
This guarantees that the behavior of the useCurrentYearIfMissing option is stable and not affected by the actual date.

Comment on lines 8 to 16
// Mock: fix current date to 2025-04-23 to test `useCurrentYearIfMissing` option
let clock;
before(() => {
clock = sinon.useFakeTimers(new Date(2025, 4, 23).getTime());
});
after(() => {
clock.restore();
});

Copy link
Member

Choose a reason for hiding this comment

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

I think that add currentYear option instead of using mocking.

{
  "useCurrentYearIfMissing": true,
  "currentYear": 2025, // default: new Date()).getFullYear()
}

As a result, you do not need mocking, just pass currentYear option.
Also, currentYear will be useful in some use-case.

Copy link
Contributor Author

@qiichos qiichos Apr 24, 2025

Choose a reason for hiding this comment

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

Thank you for your suggestion. You are absolutely right!
I’ve added the currentYear option as you recommended, so mocking is no longer necessary.
I appreciate your helpful feedback!✨

add: currentYear option and remove sinon dependency

Comment on lines 68 to 116
const chronoDates = chrono.parse(text);
let chronoDates = chrono.parse(text);
// Add current year if missing and option is enabled
if (useCurrentYearIfMissing) {
const currentYear = (new Date()).getFullYear();
chronoDates.forEach(chronoDate => {
// If year is not specified in the parsed result
if (
chronoDate.start &&
chronoDate.start.knownValues.year === undefined
) {
// Detect language for the date string
const lang = detectLang(Object.keys(chronoDate.tags), preferLang);
if (!lang) {
return;
}
// Re-parse the text with the year added
const newText = addYearToDateText(chronoDate.text, currentYear, lang);
const reparsed = chrono.parse(newText, undefined, {forwardDate: true});
// If reparsed successfully, update knownValues with year/month/day
if (reparsed && reparsed[0] && reparsed[0].start && reparsed[0].start.knownValues) {
Object.assign(chronoDate.start.knownValues, reparsed[0].start.knownValues);
}
}
});
}
Copy link
Member

@azu azu Apr 24, 2025

Choose a reason for hiding this comment

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

We want to avoid to use let and re-assign.
I think the scope of the let can be reduced by cutting it out to a function like createChronoDates.

const createChronoDates = (dateText:string, options: {
  preferLang: string;
  useCurrentYearIfMissing: boolean;
  currentYear: number
}) => { 
  let chronoDates = chrono.parse(text);
  if(!option.useCurrentYearIfMissing) {
    return chronoDates;
  }
  ... 
}

...
const chronoDates = createChronoDates(text, {
  preferLang,
  useCurrentYearIfMissing,
  currentYear
});

Copy link
Member

@azu azu left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks!

@azu azu added the Type: Feature New Feature label Apr 24, 2025
@azu azu merged commit 9a72370 into textlint-rule:master Apr 24, 2025
3 checks passed
@azu
Copy link
Member

azu commented Apr 24, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Feature New Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants