Skip to content

#217 add icao-mul-cyrl-latn-2015#570

Closed
CAMOBAP wants to merge 4 commits intomainfrom
feature/icao-mul-cyrl-latn-2015
Closed

#217 add icao-mul-cyrl-latn-2015#570
CAMOBAP wants to merge 4 commits intomainfrom
feature/icao-mul-cyrl-latn-2015

Conversation

@CAMOBAP
Copy link
Contributor

@CAMOBAP CAMOBAP commented Oct 17, 2020

  • #217

@CAMOBAP CAMOBAP requested a review from ronaldtse October 17, 2020 15:47
@CAMOBAP CAMOBAP self-assigned this Oct 17, 2020
@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Oct 17, 2020

@ronaldtse it's a first mul transliteration scheme how should we handle language-specific rules, maybe should we implement such feature and allow "override" language property in yaml config?

Proposal

Language-specific rules can look like this:

    - language: iso-639-2:bel
      pattern: .....
      result: "..."

Extend Interscript.transliterate(system_code, string, maps={}) to Interscript.transliterate(system_code, string, maps={}, options={}), and allow key language in options

@ronaldtse @AhMohsen46 @andrew2net @antonsviridenko let me know how do you think?

@ronaldtse
Copy link
Contributor

ronaldtse commented Oct 17, 2020

@CAMOBAP agree, for the “mul” map, it should be coded as “iso-639-3:mul”, and the language specific rules could be either:

  1. implemented in separate maps with separate language codes (in separate files), or
  2. we could put those rules inside the “mul” file, but we will need to find a way to keep the information accessible from the map.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Oct 17, 2020

@ronaldtse as far as I see we already implemented the first option, we have a list of map/icao-* mappings

@ronaldtse
Copy link
Contributor

ronaldtse commented Oct 18, 2020

@CAMOBAP Then is your proposal to also allow the second option?

There are indeed systems that are supposed to work with multiple languages, e.g. ISO 9 is supposed to work with 62 languages and splitting it into 62 files is not ideal.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Oct 18, 2020

Then is your proposal to also allow the second option?

I thought that this task created exactly for this approach (single mapping many supported languages) initially, correct me if I'm wrong

@ronaldtse
Copy link
Contributor

ronaldtse commented Oct 18, 2020

@CAMOBAP it wasn't but it seems like a good idea 😉

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Oct 18, 2020

@ronaldtse ok I will update this PR according to the https://github.com/interscript/interscript/pull/570#issuecomment-711034309 above

@CAMOBAP CAMOBAP marked this pull request as ready for review November 4, 2020 09:48
@CAMOBAP CAMOBAP force-pushed the feature/icao-mul-cyrl-latn-2015 branch from bf032ab to a92a617 Compare November 4, 2020 09:48
hound[bot]
hound bot reviewed Nov 4, 2020
View reviewed changes
spec/interscript_spec.rb
it "test for #{test}" do
Timeout::timeout(5) do
result = Interscript.transliterate(system_name, test["source"], cache)
result = Interscript.transliterate(system_name, test["source"], cache, { :language => test["language"] })
Copy link

@hound hound bot Nov 4, 2020

Choose a reason for hiding this comment

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

Line is too long. [117/80]
Redundant curly braces around a hash parameter.
Use the new Ruby 1.9 hash syntax.

@CAMOBAP CAMOBAP force-pushed the feature/icao-mul-cyrl-latn-2015 branch 2 times, most recently from 53f3765 to 7ede562 Compare November 4, 2020 12:43
@ronaldtse
Copy link
Contributor

ronaldtse commented Nov 4, 2020

Thanks @CAMOBAP !

@webdev778 could you have a look on how to port this functionality to the JS side? Please directly commit to this branch. Thanks!

@CAMOBAP CAMOBAP mentioned this pull request Feb 6, 2021
Open
@CAMOBAP CAMOBAP force-pushed the feature/icao-mul-cyrl-latn-2015 branch from 7ede562 to 006388f Compare June 10, 2021 11:33
hound[bot]
hound bot reviewed Jun 10, 2021
View reviewed changes
spec/interscript_spec.rb
cache = {}
# Let's preload the cache but silence the exception if not possible
# (it will be reraised during the test)
Interscript.transliterate(system_name, "", cache) rescue nil
Copy link

@hound hound bot Jun 10, 2021

Choose a reason for hiding this comment

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

Avoid using rescue in its modifier form.

spec/interscript_spec.rb

require "timeout"
require "pycall/import"
include PyCall::Import
Copy link

@hound hound bot Jun 10, 2021

Choose a reason for hiding this comment

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

include is used at the top level. Use inside class or module.

@ronaldtse
Copy link
Contributor

ronaldtse commented Jun 10, 2021

Thank you @CAMOBAP ! Finally!!

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Jun 10, 2021

@ronaldtse not done yet unfortunatelly( but I will deal with it today

@ronaldtse ronaldtse mentioned this pull request Oct 7, 2021
Open
@ronaldtse
Copy link
Contributor

ronaldtse commented Oct 7, 2021

Closed in favour of interscript/maps#168

@ronaldtse ronaldtse closed this Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronaldtse ronaldtse Awaiting requested review from ronaldtse

1 more reviewer

@hound hound[bot] hound[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@CAMOBAP CAMOBAP

@webdev778 webdev778

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@CAMOBAP @ronaldtse