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

Feature/TR-5014/Text converter #163

Merged
merged 14 commits into from
Dec 27, 2022
Merged

Conversation

jsconan
Copy link
Contributor

@jsconan jsconan commented Dec 21, 2022

Related to: https://oat-sa.atlassian.net/browse/TR-5014

Summary

Add a text converter for sanitizing text.

This first version includes a converter for replacing ambiguous symbols with plain ASCII chars.

Details

Several components have been added:

  • 'util/converter/factory' a converter factory, registering a list of processors for converting the text
  • 'util/converter/ambiguousSymbols' an ambiguous symbols processor for replacing Japanese number with ASCII numbers
  • 'util/converter' a default converter bundled with the builtin converted (for now only the ambiguousSymbols processor)

Note: the list of ambiguous symbols to replace might not be finalized yet. Additions may be done soon if we get the final list.

How to test

  • checkout the branch
  • install the package: npm ci
  • run the unit tests: npm test
  • you can also run the test from a browser'

@jsconan jsconan requested a review from shaveko December 21, 2022 13:16
@github-actions
Copy link

github-actions bot commented Dec 21, 2022

Coverage Report

Totals Coverage
Statements: 90.59% ( 2668 / 2945 )
Methods: 95% ( 608 / 640 )

@jsconan jsconan requested a review from rgomeztao December 21, 2022 13:23
Copy link
Contributor

@shaveko shaveko left a comment

Choose a reason for hiding this comment

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

Nice job! It's very good designed in that pluggable way.

I have doubts about merging of configs, could you please add test case to test/util/converter/factory/test.js for the case when we have two processors having same config property (what will be result of their merge).

src/util/converter/factory.js Outdated Show resolved Hide resolved
src/util/converter/factory.js Outdated Show resolved Hide resolved
src/util/converter/factory.js Show resolved Hide resolved
@jsconan jsconan requested review from shaveko and rgomeztao December 22, 2022 08:39
Copy link
Contributor

@shaveko shaveko left a comment

Choose a reason for hiding this comment

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

Looks nice! Good job, adorable code.

@jsconan jsconan closed this Dec 22, 2022
@jsconan jsconan deleted the feature/TR-5014/text-converter branch December 22, 2022 11:01
@jsconan jsconan restored the feature/TR-5014/text-converter branch December 22, 2022 11:01
@jsconan jsconan reopened this Dec 22, 2022
@jsconan jsconan closed this Dec 22, 2022
@jsconan jsconan deleted the feature/TR-5014/text-converter branch December 22, 2022 11:02
@jsconan jsconan restored the feature/TR-5014/text-converter branch December 22, 2022 11:02
@jsconan jsconan reopened this Dec 22, 2022
@github-actions
Copy link

Version

Target Version 1.22.0
Last version 1.21.1

There are 0 BREAKING CHANGE, 5 features, 1 fix

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.

3 participants