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

CLDR script update #487

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

CLDR script update #487

wants to merge 7 commits into from

Conversation

asadurski
Copy link
Member

  • Skip entries in locales with capitalisation differences.
  • Should also slightly influence language detection through reducing the number of options.

TODO: documentation update must follow.

@asadurski asadurski added this to the v0.7.1 milestone Jan 10, 2019
@codecov
Copy link

codecov bot commented Jan 10, 2019

Codecov Report

Merging #487 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #487   +/-   ##
=======================================
  Coverage   95.21%   95.21%           
=======================================
  Files         302      302           
  Lines        2506     2506           
=======================================
  Hits         2386     2386           
  Misses        120      120           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb79181...1ef3ebb. Read the comment docs.

@asadurski asadurski modified the milestones: v0.7.1, v0.7.2 Feb 15, 2019
@asadurski asadurski added this to To review in Project board Jun 8, 2019
@asadurski asadurski removed this from the v0.7.2 milestone Aug 28, 2019
scripts/utils.py Show resolved Hide resolved
@Gallaecio
Copy link
Member

@asadurski What else is missing here? The pull request description mentions updating the documentation, what needs to be updated there?

@@ -38,16 +43,15 @@ def _modify_relative_data(relative_data):
string = RELATIVE_PATTERN.sub(r'(\\d+)', string)
value[i] = string
modified_relative_data[key] = value
return modified_relative_data
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is not going to return the modified_relative_data dict we don't even need to create it because we don't need to track it.

suggestion: deleting modified_relative_data = OrderedDict() and modified_relative_data[key] = value.

Comment on lines +42 to +45
if not primary_dict:
return supplementary_dict
elif not supplementary_dict:
return primary_dict
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not primary_dict:
return supplementary_dict
elif not supplementary_dict:
return primary_dict
if not primary_dict or not supplementary_dict:
return primary_dict or supplementary_dict

this is simpler, but I'm not sure about the readability of my code 🤔

@noviluni
Copy link
Collaborator

noviluni commented May 1, 2020

Hi @asadurski ! 😄

I think we could merge it as-is and merge the new CLDR data in a different PR.

I'm editing the same file here to support testing it: #663
and I think it would be a good idea to merge this first and resolve the conflicts in the other PR.

What docs would you update? If you don't have time just let me know and we take this PR.

@noviluni noviluni mentioned this pull request May 2, 2020
@@ -17,18 +19,20 @@
numeral_translation_directory = '../dateparser/data/numeral_translation_data/'

os.chdir(os.path.dirname(os.path.abspath(__file__)))
logging.basicConfig(level=logging.INFO, format='%(asctime)s %(message)s')
log = logging.getLogger('data_scripts')
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you add the log here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Project board
To review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants