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

Added support for fortnight and century #987

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

Conversation

Mr-Sunglasses
Copy link
Contributor

Add support for other words ("decade", "fortnight", "century"...) #725

@Gallaecio
Copy link
Member

You seem to have accidentally deleted a file.

@Mr-Sunglasses
Copy link
Contributor Author

ohk I'm fixing them

@Mr-Sunglasses Mr-Sunglasses changed the title Added suppor for for fortnight and decade Added support for fortnight in century Sep 28, 2021
@Mr-Sunglasses Mr-Sunglasses changed the title Added support for fortnight in century Added support for fortnight and century Sep 28, 2021
@Mr-Sunglasses
Copy link
Contributor Author

@Gallaecio fixes the files , Now it seems like everything is Ok

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #987 (b461617) into master (0ed979e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b461617 differs from pull request most recent head da174a8. Consider uploading reports for the commit da174a8 to get more accurate results

@@            Coverage Diff             @@
##           master     #987      +/-   ##
==========================================
- Coverage   98.29%   98.29%   -0.01%     
==========================================
  Files         234      234              
  Lines        2702     2700       -2     
==========================================
- Hits         2656     2654       -2     
  Misses         46       46              
Impacted Files Coverage Δ
dateparser/data/date_translation_data/en.py 100.00% <ø> (ø)
dateparser/data/date_translation_data/hi.py 100.00% <ø> (ø)
dateparser/languages/dictionary.py 100.00% <ø> (ø)
dateparser/freshness_date_parser.py 99.07% <100.00%> (+0.05%) ⬆️
dateparser/date_parser.py 93.75% <0.00%> (-0.19%) ⬇️
dateparser/date.py 99.24% <0.00%> (-0.02%) ⬇️

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 0ed979e...da174a8. Read the comment docs.

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

I see tests only use “century”, in singular. I think it would be good to also cover its plural, both the grammatically correct “centuries” and the supported typo “centurys”.

@Mr-Sunglasses
Copy link
Contributor Author

Mr-Sunglasses commented Sep 28, 2021

@Gallaecio ok I'll add them on tests , Thanks for your support and guidance

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 73 to 80
in \1 century:
- in (\d+) century?
\1 century ago:
- (\d+) century? ago
in \1 fortnight:
- in (\d+) fortnight?
\1 fortnight ago:
- (\d+) fortnight? ago
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct. The ? indicates an optional letter.

Doing this it will accepts "in 1 century" but also "in 1 centur". You need to add an s before the ?.
Also, you should need to add support for centuries here.

You can check if both thigns work with this

 dateparser.parse("in 3 centurys")

and:

 dateparser.parse("in 3 centuries")

Now both are failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noviluni sure fixing the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noviluni The tests are failing for centuries , even I added them on en.yaml.

@@ -280,10 +280,10 @@
"(\\d+) decades? ago"
],
"in \\1 century": [
"in (\\d+) century?"
"in (\\d+) centurys?"
Copy link
Member

Choose a reason for hiding this comment

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

I think it needs to be something like centur(?:ys?|ies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gallaecio Just tried this , Tests are failing when I used it.

@Mr-Sunglasses
Copy link
Contributor Author

@Gallaecio - I have fixed all the changes that you were request 😊

param('last fortnight', ago={'days': 14}, period='day'),
param('14 fortnight', ago={'days': 196}, period='day'),
param('a fortnight ago', ago={'days': 14}, period='day'),
param('last fortnight', ago={'days': 14}, period='day'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is repeated and the same as in line 61.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Done this change, Please Look into it 😊.

param('last fortnight', ago={'days': 14}, period='day'),
param('14 fortnight', ago={'days': 196}, period='day'),
param('a fortnight ago', ago={'days': 14}, period='day'),
param('last fortnight', ago={'days': 14}, period='day'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. It is repeated as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Done this change, Please Look into it 😊.

@@ -841,6 +871,7 @@ def test_relative_past_dates(self, date_string, ago, period):
param('1 वर्ष, 8 महीने, 2 सप्ताह', ago={'years': 1, 'months': 8, 'weeks': 2}, period='week'),
param('1 वर्ष 7 महीने', ago={'years': 1, 'months': 7}, period='month'),
param('आज', ago={'days': 0}, period='day'),
param('1 दशक पहले', ago={'years': 10}, period='year'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a few more test cases for the Hindi version here? For e.g.

  • 1 दशक पूर्व
  • दो दशक पहले
  • 10 दशकों पहले

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Done this change, Please Look into it 😊.

@@ -1160,6 +1199,7 @@ def test_normalized_relative_dates(self, date_string, ago, period):
param('17 सेकंड बाद', in_future={'seconds': 17}, period='day'),
param('1 वर्ष, 5 महीने, 1 सप्ताह में',
in_future={'years': 1, 'months': 5, 'weeks': 1}, period='week'),
param('1 दशक में', in_future={'years': 10}, period='year'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we also add a few more test cases for the Hindi version here? For e.g.

  • पांच दशक बाद
  • दश दशक पश्चात
  • 9 दशकों मे

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Done this change, Please Look into it 😊.

@@ -1066,6 +1097,14 @@ def test_normalized_relative_dates(self, date_string, ago, period):

@parameterized.expand([
# English dates
param('in a fortnight', in_future={'days': 14}, period='day'),
param('next fortnight', in_future={'days': 14}, period='day'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure, but do we support coming fortnight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Done this change, Please Look into it 😊.

Comment on lines +371 to +372
param('1 दशक', ago={'years': 10}, period='year'),
param('1 दशक पहले', ago={'years': 10}, period='year'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have added a few suggestions above to improve the Hindi test cases. Please have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Done this change, Please Look into it 😊.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe these test cases can be added here as well

  • 1 दशक पूर्व
  • दो दशक पहले
  • 10 दशकों पहले

@@ -10,6 +10,8 @@ november:
- नवम्बर
december:
- दिसम्बर
decade:
- दशक
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can add the plural for this as well, e.g. दशकों

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Thanks For Suggestions I'll be Implementing Them ASAP ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gutsytechster Done this change, Please Look into it 😊.

@Mr-Sunglasses
Copy link
Contributor Author

@Gallaecio , @gutsytechster Please Look into it I have done all Changes.

.gitignore Outdated Show resolved Hide resolved
@Mr-Sunglasses
Copy link
Contributor Author

Mr-Sunglasses commented Jun 20, 2022

@Gallaecio Fixed the changes you requested and also add some improvements 👍🏻

@@ -51,4 +51,4 @@ docs/_build
.vscode/

# Other
raw_data
raw_data
Copy link
Member

Choose a reason for hiding this comment

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

💄 You removed the last empty line 🙂

@@ -73,7 +73,7 @@ relative-type-regex:
- (\d+) decades? ago
in \1 century:
- in (\d+) centurys?
- in (\d+) centur(?:ys?|ies)
- in (\d+) centuries?
Copy link
Member

@Gallaecio Gallaecio Jun 21, 2022

Choose a reason for hiding this comment

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

You can remove the ?. And add "(\\d+) centuries ago" below.

Copy link
Collaborator

@gutsytechster gutsytechster left a comment

Choose a reason for hiding this comment

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

To confirm that you have added the following support

  1. fortnight support in English
  2. century support in English
  3. decade support in Hindi

Comment on lines +225 to +226
"(\\d+) दशक मे",
"(\\d+) दशकों मे",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't these same as the first two expressions?

"बाद"
"बाद",
"पश्चात",
"मे"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already present as the first item on this list.

@@ -29,7 +32,44 @@ ago:
in:
- में
- बाद
- पश्चात
- मे
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is repeated as well.

Comment on lines +50 to +51
- (\d+) दशक मे
- (\d+) दशकों मे
Copy link
Collaborator

Choose a reason for hiding this comment

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

Repeated as the above two expressions.

- (\d+) दशक पहले
- (\d+) दशकों पहले


Copy link
Collaborator

Choose a reason for hiding this comment

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

just a nitpick: there is an extra line here.

- दस: '10'
- दश: '10'
- ग्यारह: '11'
- बारह: '12'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to add a new line here.

Comment on lines +371 to +372
param('1 दशक', ago={'years': 10}, period='year'),
param('1 दशक पहले', ago={'years': 10}, period='year'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe these test cases can be added here as well

  • 1 दशक पूर्व
  • दो दशक पहले
  • 10 दशकों पहले

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.

None yet

4 participants