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: add more arabic names dataset #362

Merged
merged 1 commit into from
Jan 30, 2022

Conversation

wael-fadlallah
Copy link
Contributor

No description provided.

@wael-fadlallah wael-fadlallah requested a review from a team as a code owner January 29, 2022 21:50
@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent labels Jan 29, 2022
@Shinigami92 Shinigami92 added this to the v6.2 - New small features milestone Jan 29, 2022
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.

Formatting looks good. I cannot verify the actual translations though.

Copy link
Contributor

@JessicaSachs JessicaSachs left a comment

Choose a reason for hiding this comment

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

Yeah we can't validate the translations except for using Google Translate.

@JessicaSachs JessicaSachs merged commit 86f1208 into faker-js:main Jan 30, 2022
@Shinigami92
Copy link
Member

Yeah we can't validate the translations except for using Google Translate.

@JessicaSachs Was it expected to merge something into main that was targeted for v6.2?
We currently highly try only to focus on absolutely needed project stability changes.

@@ -0,0 +1,13 @@
export default [
'فاطمه',
Copy link
Member

Choose a reason for hiding this comment

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

This and the other files are not formatted at all and now causing CI errors. We need 3 maintainers to fix the main branch. 1 that creates the PR and two approvals.
Until then we cannot rebase any of the other PRs anymore 😬

@wael-fadlallah
Copy link
Contributor Author

@Shinigami92 This could be considered a stability change, the reason I add these files is when I used AR locale the findName Method returns an English first name with an Arabic last name, I tried to debug the actual issue but I couldn't figure it out but adding these files returns a valid Arabic name

@Shinigami92
Copy link
Member

@Shinigami92 This could be considered a stability change, the reason I add these files is when I used AR locale the findName Method returns an English first name with an Arabic last name, I tried to debug the actual issue but I couldn't figure it out but adding these files returns a valid Arabic name

I know, sorry if we did not clarify that to much right now, but we maintainers are currently working daily multiple hours on rewriting e.g. the whole test suite and introduce linting and such.
Adding a new locale is definitely planned and wanted. But not right now.

Internally we plan what we are currently focus on, and more and more code on top of what we currently need to fix and stabilize is currently not helping.

Sorry, I already work like 10h per day currently on the Code base. I can really do not more right now to speedup the process.

@ST-DDT
Copy link
Member

ST-DDT commented Jan 30, 2022

@wael-fadlallah Could you please create a new PR with your changes?
Your contributions are appreciated and we are sorry for the trouble/additional work we caused you.

@wael-fadlallah
Copy link
Contributor Author

@ST-DDT No need for apologies I'm happy to help, just to make sure everything will go smoothly this time can I know what is the formatting issue that caused troubles in my first PR

@Shinigami92
Copy link
Member

Shinigami92 commented Jan 30, 2022

Yeah for sure, it was wrongly indented lines and the trailing +-es

You don't need to care for these yourself, just run pnpm run format or do not bypass the lint-stage that should prevent such commits

You can also see now lint errors in the files changes tab: https://github.com/faker-js/faker/pull/362/files

bmenant pushed a commit to bmenant/faker that referenced this pull request Mar 11, 2022
bmenant pushed a commit to bmenant/faker that referenced this pull request Mar 11, 2022
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants