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

feat: ✨ support for polish language #189

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

Conversation

sectasy0
Copy link

@sectasy0 sectasy0 commented Jun 30, 2022

I add polish support and some test with correct fixtures examples. I don't know if I did something wrong, polish support works almost fine besides a few cases that I mentioned in Issue.

Closes #189.

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Fix CI, update CHANGELOG and README please? rubocop -a will go a long way

@sectasy0 sectasy0 requested a review from dblock July 1, 2022 09:46
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks like you have some CR/LF changes that sneaked in. Make sure rubocop passes locally and you might want to configure git/your editor to match unix CR/LF.

See also version changes below. Let's make this a 0.12.0 change.

CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
## Unreleased

### Features
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go under Next below, that's our unreleased.

@@ -1,5 +1,5 @@
# frozen_string_literal: true

module NumbersAndWords
VERSION = '0.11.12'
VERSION = '0.11.11'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was correct, next version is 0.11.12 and the current code is incremented for that.

I do think we should make next version 0.12.0 since we're adding a whole new language. Please change this and the version in CHANGELOG to 0.12.0.

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 still incorrect.

end
end
end
# frozen_string_literal: true
Copy link
Collaborator

@dblock dblock Jul 3, 2022

Choose a reason for hiding this comment

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

CR/LF changes - you should probably just revert it, and ensure that new files have unix CR/LF

same for the other two in this directory

@sectasy0 sectasy0 requested a review from dblock July 21, 2022 14:20
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

See comments for unnecessary version changes (leave versions alone and this is good to go).

CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
## Unreleased
Copy link
Collaborator

Choose a reason for hiding this comment

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

0.11.12 wasn't released, move your feature to the section below

@sectasy0 sectasy0 requested a review from dblock July 21, 2022 18:23
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Thanks for hanging in here! There's still a couple of changes below that need to be reverted and tests are failing.

https://github.com/kslazarev/numbers_and_words/runs/7460137123?check_suite_focus=true

README.rdoc Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
# frozen_string_literal: true

module NumbersAndWords
VERSION = '0.11.12'
VERSION = '0.11.11'
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 still incorrect.

CHANGELOG.md Show resolved Hide resolved
@sectasy0 sectasy0 requested a review from dblock July 22, 2022 08:41
@sectasy0
Copy link
Author

sectasy0 commented Jul 22, 2022

There is still a problem with hundreds and a few other things, maybe it's a configuration issue in the strategies / figures_converter/languages/pl.rb file. I don't know how to solve it, I added some tests with correct examples. Most of the things are practically similar to what in Czech I tried to copy the content from upload to pl.rb but it was throwing me errors.

NoMethodError: undefined method result' for nil:NilClass - options.gender.result`

Maybe with your help I can figure out this problem.

@dblock
Copy link
Collaborator

dblock commented Jul 23, 2022

@sectasy0 if you just copy over all of translations/cs.rb onto translations/pl.rb you'll get quite a bit further. At least it will get rid of mechanical errors. Unfortunately I think one needs to speak Polish to properly construct the translation rules around gender/ordinals/etc.

@sectasy0
Copy link
Author

@dblock I'm native polish speaker. All translations are properly constructed in .yml files as well as correct examples.

Example with hundreds:

 expected: "dziewięćset dziewiętnaście"
             got: "dziewięć _ sto dwieście trzysta czterysta pięćset sześćset siedemset osiemset dziewięćset dziewiętnaście"

@dblock
Copy link
Collaborator

dblock commented May 14, 2024

@sectasy0 want to finish this?

@sectasy0
Copy link
Author

@sectasy0 want to finish this?

Yeah, but I don't quite understand why it returns all the things, I just copied everything from czech language which is basically the same as in Polish in terms of numbers, wanna help me a little?

@dblock
Copy link
Collaborator

dblock commented May 15, 2024

I checked out your code and rn all specs pass. Add ones that fail and I can try to take a look?

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.

2 participants