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.

Sign up for GitHub

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

Jump to bottom

feat(lorem): seed AR lorem #2147

Merged
merged 14 commits into from
Aug 4, 2023
Merged

feat(lorem): seed AR lorem #2147

merged 14 commits into from
Aug 4, 2023

Conversation

ahmedrowaihi
Copy link
Contributor

No description provided.

@ahmedrowaihi ahmedrowaihi requested a review from a team as a code owner May 10, 2023 15:18
@ST-DDT
Copy link
Member

ST-DDT commented May 10, 2023

Please run pnpm run preflight.
Also could you please explain how you have chosen the values for lorem?
E.g. are these actual words or just translations of the original lorems or random character sequences ?

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: awaiting more info Additional information are requested c: locale Permutes locale definitions m: lorem Something is referring to the lorem module labels May 10, 2023
@ahmedrowaihi
Copy link
Contributor Author

Please run pnpm run preflight. Also could you please explain how you have chosen the values for lorem? E.g. are these actual words or just translations of the original lorems or random character sequences ?

YES, @ST-DDT

  • I Did run pnpm run preflight
  • They are actual words, I tested them in my vs-code extension lorem-ipsum-ar, and as an Arab myself, the generated texts are satisfying.

@codecov
Copy link

codecov bot commented May 10, 2023 •
edited
Loading

Codecov Report

Merging #2147 (3238123) into next (02fc7ca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@           Coverage Diff            @@
##             next    #2147    +/-   ##
========================================
  Coverage   99.60%   99.60%            
========================================
  Files        2641     2643     +2     
  Lines      244831   245738   +907     
  Branches     1084     1083     -1     
========================================
+ Hits       243852   244760   +908     
+ Misses        952      951     -1     
  Partials       27       27
Files Changed Coverage Δ
src/locales/ar/index.ts 100.00% <100.00%> (ø)
src/locales/ar/lorem/index.ts 100.00% <100.00%> (ø)
src/locales/ar/lorem/words.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@matthewmayer
Copy link
Contributor

Not directly related to this PR, but whats the purpose of the "supplemental" vs "words" list? It seems the "supplemental" list is not used. Should they be merged into one?

@matthewmayer matthewmayer changed the title feat(locales/ar): seed AR lorem feat(lorem): seed AR lorem May 11, 2023
@ahmedrowaihi
Copy link
Contributor Author

Not directly related to this PR, but whats the purpose of the "supplemental" vs "words" list? It seems the "supplemental" list is not used. Should they be merged into one?

I just mimicked the EN and other locales lorem, but I see your point, even in the src/definitions/lorem.ts I don't see supplements defined.

@matthewmayer matthewmayer mentioned this pull request May 12, 2023
Closed
@xDivisionByZerox xDivisionByZerox removed the s: awaiting more info Additional information are requested label May 14, 2023
@ahmedrowaihi
Copy link
Contributor Author

@xDivisionByZerox what info missing ?

@xDivisionByZerox
Copy link
Member

I removed the label that was previously added.
Previously @ST-DDT asked for more information which you already answered.

So nothing to worry.

@ahmedrowaihi
Copy link
Contributor Author

I removed the label that was previously added. Previously @ST-DDT asked for more information which you already answered.

So nothing to worry.

Ahh I see, my bad, I haven't noticed that label was added.
Paradon me @xDivisionByZerox

ST-DDT
ST-DDT requested changes Jun 17, 2023
View reviewed changes
src/locales/ar/lorem/supplemental.ts Outdated Show resolved Hide resolved
@ahmedrowaihi
Copy link
Contributor Author

Here is my update, I removed all negative connotations :'D
@matthewmayer @xDivisionByZerox

@ST-DDT
Copy link
Member

ST-DDT commented Jul 20, 2023

In #2242 we merged supplemental into the lorem words file.

Could you please do that here as well?

@ahmedrowaihi
Copy link
Contributor Author

Sure!

@ahmedrowaihi
Copy link
Contributor Author

@ST-DDT Here you go!

ST-DDT
ST-DDT reviewed Jul 22, 2023
View reviewed changes
src/locales/ar/lorem/words.ts Show resolved Hide resolved
src/locales/ar/lorem/words.ts Outdated Show resolved Hide resolved
src/locales/ar/lorem/words.ts Outdated Show resolved Hide resolved
@ahmedrowaihi ahmedrowaihi requested a review from ST-DDT July 23, 2023 12:55
ST-DDT
ST-DDT approved these changes Jul 31, 2023
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Checked via google translate. Looks good to me.

@ST-DDT ST-DDT requested review from a team July 31, 2023 21:45
@ahmedrowaihi
Copy link
Contributor Author

When is this going to be merged🥹, it's been 3 months fam, haven't felt happy for couple times

@ST-DDT ST-DDT merged commit 6137801 into faker-js:next Aug 4, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Aug 4, 2023

Sorry for the delay with the reviews.
It is currently holiday season and we are all busy.

@ahmedrowaihi
Copy link
Contributor Author

Thank you all!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewmayer matthewmayer matthewmayer left review comments

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@ST-DDT ST-DDT ST-DDT approved these changes

Assignees

@ahmedrowaihi ahmedrowaihi

Labels
c: locale Permutes locale definitions m: lorem Something is referring to the lorem module p: 1-normal Nothing urgent
Projects
No open projects
Faker Roadmap
Status: In Progress
Milestone
v8.x
Development

Successfully merging this pull request may close these issues.
5 participants
@ahmedrowaihi @ST-DDT @matthewmayer @xDivisionByZerox @Shinigami92