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

Update dependencies #13

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tyler36
Copy link

@tyler36 tyler36 commented Nov 8, 2023

This PR updates package and workflow dependencies.

Fixes #2
Fixes Dependabot CVE-2021-23371

@tyler36 tyler36 marked this pull request as draft November 8, 2023 08:22
@tyler36 tyler36 marked this pull request as ready for review November 8, 2023 08:47
@tyler36
Copy link
Author

tyler36 commented Nov 8, 2023

The tests are not passing locally. I get

AssertionError [ERR_ASSERTION]: Could not specify options property in invalid object when TestConfig was passed. Use TestConfig.rules.options.
    at /home/user13/code/pr/textlint-rule-date-weekday-mismatch/node_modules/textlint-tester/src/textlint-tester.ts:410:28

Comment on lines -5 to +14
const rule = require("../src/textlint-rule-date-weekday-mismatch");
const dateWeekdayMismatchRule = require("../src/textlint-rule-date-weekday-mismatch");
const rule = {
rules: [{
ruleId: "date-weekday-mismatch",
rule: dateWeekdayMismatchRule,
options: {
lang: "ja"
}
}]
}
Copy link
Member

@azu azu Nov 8, 2023

Choose a reason for hiding this comment

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

Was there a problem with passing the rule as is?
Most test cases test if no options = automatic determination works well..

Copy link
Member

@azu azu Nov 8, 2023

Choose a reason for hiding this comment

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

const rule = {...} is for multiple rules or plugin combination.

Copy link
Author

Choose a reason for hiding this comment

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

After updating the dependencies, the tests failed:

+ '2016年12月29日(月)'
- '2016年12月29日(木)'
               ^
      + expected - actual

      -2016年12月29日(月)
      +2016年12月29日(木)

I assumed this was due to needing the language set?

Copy link
Author

@tyler36 tyler36 Nov 9, 2023

Choose a reason for hiding this comment

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

English also failing:

+ actual - expected

+ '2021-07-02 (Thursday)'
- '2021-07-02 (Friday)'

@azu
Copy link
Member

azu commented Nov 8, 2023

AssertionError [ERR_ASSERTION]: Could not specify options property in invalid object when TestConfig was passed. Use TestConfig.rules.options.

This error is occured when rule's options is conflict.

const rule = {
    rules: [{
        ruleId: "date-weekday-mismatch",
        rule: dateWeekdayMismatchRule,
        options: {
            lang: "ja" 
        }
    }]
}

vs.

        {
            text: "2016-12-29(月)",
            output: "2016-12-29(木)",
            options: {
                lang: "ja-JP"
            },
        }

textlint-tester can not override options.

@tyler36
Copy link
Author

tyler36 commented Nov 9, 2023

options: { lang: "ja-JP" },

The readme on https://github.com/textlint-rule/textlint-rule-date-weekday-mismatch lists Japanese as ja in "Supported lang" and first "Options". Reading more closely, I see ja-JP as the second option, but ja below that in the ISO 639-1 e.g. I am confused.

@azu
Copy link
Member

azu commented Nov 9, 2023

https://github.com/wanasit/chrono/releases/tag/v2.0.2

Chrono’s default now handles only international English. While in the previous version, it tried to parse with all known languages.

// default English (US)
chrono.parseDate('6/10/2018');
https://github.com/wanasit/chrono#locales

chrono v2 seems to have lost the automatic language determination feature.

@azu
Copy link
Member

azu commented Nov 9, 2023

options: { lang: "ja-JP" },

I think it is intentionally vague. But I don't remember why.
Either way, it is not a problem to change the lang option since the BREAKING CHANGE is included.

@tyler36
Copy link
Author

tyler36 commented Nov 10, 2023

OK ... so src/textlint-rule-date-weekday-mismatch.js:67 in I can see this is working.
I am not sure how to get the locale string into this function.

// src/textlint-rule-date-weekday-mismatch.js:67

const text = "2016年12月29日(月)"
const chronoDates = chrono.parse(text);
// []

const chronoDates = chrono['ja'].parse(text);
// 0: ParsingResult {text: '2016年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.

Fix range must count the current text length, not the replacement's
2 participants