Pu/issue 130

[izulin]

Context

Abstracted string comparison using locale implementations. Passing some parameters from config there. Introduced more parameters (like: accentsSensitive, ignorePunctuation)

How has this been tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature or improvement (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Additional language file or change to the existing one (translations)

Related issue(s):

1. String comparison operators #130

Checklist:

• My code follows the code style of this project,
• My change requires a change to the documentation,
• I described the modification in the CHANGELOG.md file.

[izulin]

@wojciechczerniak I have run into some weird issue, where https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator/Collator is not behaving like promised. Further investigation is required, but I am initiating the discussion on whether code refactor is desired, and config parameters are done properly.

[wojciechczerniak]

@wojciechczerniak I have run into some weird issue, where https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator/Collator is not behaving like promised. Further investigation is required, but I am initiating the discussion on whether code refactor is desired, and config parameters are done properly.

It might be a Node.js issue. Locales were optional and not a part of the default Node build before v13 nodejs/node@1a25e90

We may have to wait for #71

[izulin]

@wojciechczerniak I have run into some weird issue, where https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator/Collator is not behaving like promised. Further investigation is required, but I am initiating the discussion on whether code refactor is desired, and config parameters are done properly.

It might be a Node.js issue. Locales were optional and not a part of the default Node build before v13 nodejs/node@1a25e90

We may have to wait for #71

Ok. I'll add tests, right now it seems to me that we can have control over accent-sensitivity and case-sensitivity, but not over the order of sorting (locale-dependent).

[wojciechczerniak]

Looks like the #71 is now confirmed to work on v13. Have you chacked latest Node with localeCompare?

[izulin]

Looks like the #71 is now confirmed to work on v13. Have you chacked latest Node with localeCompare?

ok, after update of node, it works as desired

[izulin]

but remote config needs updating

[wojciechczerniak]

We need to update
https://github.com/handsontable/hyperformula/blob/develop/.nvmrc
and
https://github.com/handsontable/hyperformula/tree/develop/.github/workflows

We should add this to basic requirements in docs/readme @scarletfog

[izulin]

We need to update
https://github.com/handsontable/hyperformula/blob/develop/.nvmrc
and
https://github.com/handsontable/hyperformula/tree/develop/.github/workflows

We should add this to basic requirements in docs/readme @scarletfog

I don't know how to properly edit the latter one.

[wojciechczerniak]

Ok

@budnix could you help with GH Actions? ☝️

[izulin]

Ok

@budnix could you help with GH Actions? ☝️

bump

[wojciechczerniak]

GH Actions have an issue that @swistak35 mentioned #71 (comment)

Also, an update has to be checked on Windows. An upgrade has failed for now #232

[wojciechczerniak]

@izulin It looks that CI update to Node v13 is not a trivial task. There is no prebuilt package for non-LTS node versions, which takes too much time to build the required packages from the source.

With @swistach we've found a different approach to this problem. There is a snipped in Node docs how we can detect the ICU support https://nodejs.org/docs/latest-v13.x/api/intl.html#intl_detecting_internationalization_support

Then we can conditionally skip the test:

it('blah blah', () => {
   if (hasFullICU) {

   } else {
     console.warn('You're running tests without ICU support. String comparison may work differently. Please, use Node v13+.')
   }
}

This way our CI will skip the test, while locally it will pass as long as we work on the correct Node version. We can safely move forward with the development.

The test will just start to work after we update CI to the Node v14 LTS in April. WDYT?

[izulin]

@izulin It looks that CI update to Node v13 is not a trivial task. There is no prebuilt package for non-LTS node versions, which takes too much time to build the required packages from the source.

With @swistach we've found a different approach to this problem. There is a snipped in Node docs how we can detect the ICU support https://nodejs.org/docs/latest-v13.x/api/intl.html#intl_detecting_internationalization_support

Then we can conditionally skip the test:

it('blah blah', () => {
   if (hasFullICU) {

   } else {
     console.warn('You're running tests without ICU support. String comparison may work differently. Please, use Node v13+.')
   }
}

This way our CI will skip the test, while locally it will pass as long as we work on the correct Node version. We can safely move forward with the development.

The test will just start to work after we update CI to the Node v14 LTS in April. WDYT?

Awesome!

[wojciechczerniak]

LGTM 👍