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

fix: updates for Basque (eu) #3749

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

eillarra
Copy link

As a reference you can take a look at moment.js, which was updated years ago by me: https://github.com/moment/moment/blob/develop/locale/eu.js

For example, "en {result}" was still in Spanish...

eillarra and others added 2 commits March 29, 2024 22:49
As a reference you can take a look at moment.js, which was updated years ago by me: https://github.com/moment/moment/blob/develop/locale/eu.js

For example, "en {result}" was still in Spanish...
Copy link
Member

@fturmel fturmel left a comment

Choose a reason for hiding this comment

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

I don't speak the language, but the changes look good to me!

@fturmel
Copy link
Member

fturmel commented Mar 31, 2024

@eillarra can you confirm that there are no unintended behavior changes in the updated snapshot? d97aa76

@eillarra
Copy link
Author

eillarra commented Apr 1, 2024

@fturmel snapshot looks good. There is only one thing that it is not perfect, but I don't think it is possible with the current implementation:

6 urte gutxi gorabehera barru should be 6 urte barru gutxi gorabehera

But suffix is added after result is calculated, and the 'about' (gutxi gorabehera) is added beforehand to the result, so I guess it needs to stay like this. Thanks!

@eillarra
Copy link
Author

eillarra commented Apr 1, 2024

Keep this on hold for a moment... I only checked formatDistance, but I will check the other files too so the revision is complete. There might be a couple of small extra issues that can be solved.

@fturmel
Copy link
Member

fturmel commented Apr 1, 2024

Great! Here's the unicode CLDR reference if needed: https://www.unicode.org/cldr/charts/44/summary/eu.html

If you change anything else, you can update the snapshot markdown file yourself by running npm run locale-snapshots.

I'm sure it's possible to reshape the formatDistance function to support the "about" form you're describing. How does this look?

if (options?.addSuffix) {
  if (options.comparison && options.comparison > 0) {
    if (token.startsWith("about")) {
      return result.replace("gutxi", "barru gutxi");
    }
    return result + " barru";
  } else {
    return "duela " + result;
  }
}

@fturmel fturmel marked this pull request as draft April 1, 2024 11:33
@eillarra
Copy link
Author

eillarra commented May 30, 2024

@fturmel Thank you for your patience. I finally found a moment to go through all files yesterday. I added several small changes to conform to the official Basque rules.

Snapshot has been updated too and looks OK.

@eillarra eillarra marked this pull request as ready for review May 30, 2024 08:48
@eillarra eillarra changed the title fix: formatDistance for Basque (eu) fix: updates for Basque (eu) May 30, 2024
@eillarra eillarra force-pushed the patch-1 branch 2 times, most recently from a9cfd31 to 2e12e33 Compare May 30, 2024 10:05
This fixes things like repeated years in long format and required dots in abbreviations.
medium: "y MMM d",
short: "yy/MM/dd",
short: "y-MM-dd",
Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

https://www.unicode.org/cldr/charts/44/summary/eu.html#57dac0d1b36c1261 is probably the CLDR field to look at for this value (gregorian date format short yy/M/d)

Copy link
Author

@eillarra eillarra May 30, 2024

Choose a reason for hiding this comment

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

I still think y-MM-dd will be a better choice, but I can change it to y/MM/dd if you prefer.

According to the Academy of the Basque Language (Euskaltzaindia), both are valid:

image

Copy link
Member

Choose a reason for hiding this comment

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

Fair. It's hard for a non-speaker to take a stance when things deviate from the CLDR. Maybe @date-fns/I18n-eu can chime in (and @eillarra should probably be added to this team).

@eillarra eillarra requested a review from fturmel June 4, 2024 15:46
Copy link
Member

@fturmel fturmel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Up to @kossnocorp to approve and merge, provided there are no additional comments from the i18n-eu team.

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

Successfully merging this pull request may close these issues.

None yet

2 participants