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

chore: remove broken source urls found in some locales #1511

Merged
merged 4 commits into from
Nov 9, 2022

Conversation

subbe
Copy link
Contributor

@subbe subbe commented Nov 3, 2022

Updated date files to remove the broken source links found in the following locales:

  • ar
  • az
  • cz
  • de
  • en
  • es
  • fa
  • fr
  • fr_CH
  • he
  • hr
  • mk
  • nl
  • pt_BR
  • pt_PT
  • ru
  • sv
  • vi

@subbe subbe requested a review from a team as a code owner November 3, 2022 08:37
src/locales/es/date/month.ts Show resolved Hide resolved
src/locales/es/date/weekday.ts Show resolved Hide resolved
@ST-DDT ST-DDT added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent m: date Something is referring to the date module labels Nov 3, 2022
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Nov 3, 2022
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #1511 (92a3203) into next (f1f4ad4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1511   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files        2214     2214           
  Lines      238758   238725   -33     
  Branches     1015     1016    +1     
=======================================
- Hits       237902   237872   -30     
+ Misses        835      832    -3     
  Partials       21       21           
Impacted Files Coverage Δ
src/locales/ar/date/month.ts 100.00% <ø> (ø)
src/locales/ar/date/weekday.ts 100.00% <ø> (ø)
src/locales/az/date/month.ts 100.00% <ø> (ø)
src/locales/az/date/weekday.ts 100.00% <ø> (ø)
src/locales/cz/date/month.ts 100.00% <ø> (ø)
src/locales/cz/date/weekday.ts 100.00% <ø> (ø)
src/locales/de/date/month.ts 100.00% <ø> (ø)
src/locales/de/date/weekday.ts 100.00% <ø> (ø)
src/locales/en/date/month.ts 100.00% <ø> (ø)
src/locales/en/date/weekday.ts 100.00% <ø> (ø)
... and 26 more

@ST-DDT ST-DDT requested review from a team November 3, 2022 09:01
@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Nov 3, 2022

I mean, the source comment was there for a reason...if we remove it from the locales the maintainer team should decide on an alternative strategy to provide intel on how faker data is populated (what qualifies to be taken into a source set).

@ST-DDT
Copy link
Member

ST-DDT commented Nov 3, 2022

@xDivisionByZerox It is not about removing all source links. These links are broken and I don't have any hope of any of them coming back online.

@xDivisionByZerox
Copy link
Member

@xDivisionByZerox It is not about removing all source links. These links are broken and I don't have any hope of any of them coming back online.

Ah, ok, my bad. That intend wasn't clear for me from the PRs description.

@ST-DDT
Copy link
Member

ST-DDT commented Nov 3, 2022

@xDivisionByZerox It is not about removing all source links. These links are broken and I don't have any hope of any of them coming back online.

Ah, ok, my bad. That intend wasn't clear for me from the PRs description.

I updated it to make it more clear.

@ST-DDT ST-DDT requested a review from a team November 3, 2022 21:59
@ST-DDT ST-DDT changed the title chore: remove the source url found in some locales chore: remove broken source urls found in some locales Nov 3, 2022
@matthewmayer
Copy link
Contributor

Rather than completely remove these comments, perhaps it could just be replaced with

Source: Unicode CLDR, version 27 https://unicode.org/Public/cldr/27/

That keeps the data attributed, and if someone wants to come along and update to a newer version of CLDR later, they can.

@pkuczynski
Copy link
Member

Rather than completely remove these comments, perhaps it could just be replaced with

Source: Unicode CLDR, version 27 https://unicode.org/Public/cldr/27/

Good idea!

@ST-DDT
Copy link
Member

ST-DDT commented Nov 4, 2022

My main problem with that is that it only links to a few zip files and it is not clear which file to actually look into. So at least we have to point to a specific zip with the actual file inside the zip added in parenthesis.
But if we can keep the link working then that would be good.

@matthewmayer
Copy link
Contributor

more recent versions of CLDR are in Github, e.g. https://github.com/unicode-org/cldr/blob/main/common/main/de.xml

so someone could probably go cross-check and restore correct line numbers later (or write a script to grab data directly from CLDR).

@ST-DDT
Copy link
Member

ST-DDT commented Nov 4, 2022

so someone could probably go cross-check and restore correct line numbers later (or write a script to grab data directly from CLDR).

IMO this should be done immediately using githup perma links.
However, I would be fine for a working link into the old release zips, since the actual data are probably from those versions as well.

But we should keep that idea in mind/create an issue for updating the calendar entries if possible.

@matthewmayer
Copy link
Contributor

im pretty sure a lot of the line numbers are wrong anyway, for example many languages have a comment with en.xml but it shouldn't be en.xml it should be sv.xml, mk.xml etc depending on the language. Probably some copy-pasting between locales.

@pkuczynski
Copy link
Member

I would use the links to GitHub and create a ticket to fetch it via script scheduled in CI

@matthewmayer
Copy link
Contributor

if someone wants to take that up in future note there's a JSON version generated from the XML versions which is probably easier to consume. https://github.com/unicode-org/cldr-json

@ST-DDT ST-DDT enabled auto-merge (squash) November 9, 2022 17:36
@ST-DDT ST-DDT merged commit 5a09c89 into faker-js:next Nov 9, 2022
@subbe subbe deleted the remove-url-from-date branch November 10, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior m: date Something is referring to the date module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants