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(word): add sample method #714

Merged
merged 17 commits into from
Nov 10, 2022
Merged

feat(word): add sample method #714

merged 17 commits into from
Nov 10, 2022

Conversation

pkuczynski
Copy link
Member

Fixes #276

Fixes #339

@pkuczynski pkuczynski requested a review from a team as a code owner March 28, 2022 20:35
@pkuczynski pkuczynski self-assigned this Mar 28, 2022
@pkuczynski pkuczynski added the s: accepted Accepted feature / Confirmed bug label Mar 28, 2022
@codecov
Copy link

codecov bot commented Mar 28, 2022 •
edited
Loading

Codecov Report

Merging #714 (de9f2d6) into next (666ff02) will decrease coverage by 0.00%.
The diff coverage is 92.40%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next     #714      +/-   ##
==========================================
- Coverage   99.64%   99.63%   -0.01%     
==========================================
  Files        2214     2214              
  Lines      238741   238813      +72     
  Branches     1023     1029       +6     
==========================================
+ Hits       237885   237951      +66     
- Misses        835      841       +6     
  Partials       21       21
Impacted Files Coverage Δ
src/modules/word/index.ts 98.47% <92.00%> (-1.53%) ⬇️
src/modules/helpers/index.ts 98.51% <100.00%> (ø)
src/modules/system/index.ts 100.00% <100.00%> (ø)

ST-DDT
ST-DDT requested changes Mar 28, 2022
View reviewed changes
src/word.ts Outdated Show resolved Hide resolved
src/word.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the c: feature Request for new feature label Mar 28, 2022
@ST-DDT ST-DDT added this to the v6.2 - New small features milestone Mar 28, 2022
@ST-DDT ST-DDT requested review from a team March 28, 2022 22:33
Shinigami92
Shinigami92 requested changes Mar 29, 2022
View reviewed changes
src/random.ts Outdated Show resolved Hide resolved
src/word.ts Outdated Show resolved Hide resolved
test/word.spec.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 mentioned this pull request Mar 29, 2022
Closed
@pkuczynski pkuczynski requested a review from Shinigami92 March 29, 2022 09:48
ST-DDT
ST-DDT requested changes Mar 29, 2022
View reviewed changes
src/word.ts Outdated Show resolved Hide resolved
@pkuczynski pkuczynski requested a review from ST-DDT March 29, 2022 10:54
ST-DDT
ST-DDT requested changes Mar 29, 2022
View reviewed changes
src/word.ts Outdated Show resolved Hide resolved
@pkuczynski pkuczynski requested a review from ST-DDT March 29, 2022 11:28
ST-DDT
ST-DDT previously approved these changes Mar 29, 2022
View reviewed changes
@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Mar 29, 2022
@ST-DDT ST-DDT requested a review from a team March 29, 2022 12:37
@ST-DDT ST-DDT mentioned this pull request Mar 29, 2022
Closed
@pkuczynski
Copy link
Member Author

@Shinigami92 anything else missing to get this merged?

@Shinigami92
Copy link
Member

Yes, all the v6.1 targeted things to be merged I assume 🤔

@pkuczynski
Copy link
Member Author

Yes, all the v6.1 targeted things to be merged I assume 🤔

Not sure if we should be so stick to the 6.1 is only bug fixes as keeping PRs open for longer time means potential conflicts and maintenance, so I would be in favor of merging things as soon as they ready...

@ST-DDT
Copy link
Member

ST-DDT commented Apr 1, 2022 •
edited
Loading

Yes, all the v6.1 targeted things to be merged I assume 🤔

Not sure if we should be so stick to the 6.1 is only bug fixes as keeping PRs open for longer time means potential conflicts and maintenance

That is true. Is is a small feature, requiring low effort for maintaining the PR from you.

so I would be in favor of merging things as soon as they ready...

If we merge this one, then we have to explain why we didn't merge other features.
Sure we could start merging them all, but then everyone would start creating new PRs with new super fancy features
and we have to somehow interweave the required bug fix merges with the nice to have feature requests.
Also people tend to prefer adding features over fixing bugs.

We use the milestones as a means to communicate the order we discussed on when to tackle things/fix/add...
This feature being in v6.2 has been discussed/communicated a few months ago, we don't want to discourage anyone in contributing to the project, so we don't ban PRs for future milestones, but I hope you can understand that this may cause you additional work to maintain them for the time being.

We hope to finish all fixes/PRs for 6.1 by this Monday evening, so you don't have to maintain it forever either.

@pkuczynski
Copy link
Member Author

Fair enough :)

@ST-DDT ST-DDT mentioned this pull request Apr 4, 2022
Merged
@ST-DDT
Copy link
Member

ST-DDT commented Apr 4, 2022

To consider: Handle "empty" locale
This method should at least not behave worse than the other word methods.
Ref: #770 (comment)

Shinigami92
Shinigami92 requested changes Apr 6, 2022
View reviewed changes
src/random.ts Outdated Show resolved Hide resolved
src/word.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Apr 19, 2022
@ST-DDT ST-DDT removed the deprecation A deprecation was made in the PR label Nov 3, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Nov 4, 2022

Done. Ready for Review.

ST-DDT
ST-DDT previously approved these changes Nov 4, 2022
View reviewed changes
@ST-DDT ST-DDT requested review from Shinigami92, a team, damienwebdev, import-brain, ST-DDT and xDivisionByZerox and removed request for ST-DDT November 4, 2022 00:13
import-brain
import-brain previously approved these changes Nov 4, 2022
View reviewed changes
Shinigami92
Shinigami92 reviewed Nov 4, 2022
View reviewed changes
src/modules/random/index.ts Outdated Show resolved Hide resolved
src/modules/word/index.ts Outdated Show resolved Hide resolved
src/modules/word/index.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 mentioned this pull request Nov 4, 2022
Merged
@ST-DDT @Shinigami92
chore: apply suggestions
23bfe6d 
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@Shinigami92 Shinigami92 merged commit 3777c44 into faker-js:next Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@import-brain import-brain import-brain approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

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

@damienwebdev damienwebdev Awaiting requested review from damienwebdev

@xDivisionByZerox xDivisionByZerox Awaiting requested review from xDivisionByZerox

Assignees

@pkuczynski pkuczynski

@ST-DDT ST-DDT

@damienwebdev damienwebdev

Labels
c: feature Request for new feature m: word Something is referring to the word module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.x
Development

Successfully merging this pull request may close these issues.

Move random.word to word.any
6 participants
@pkuczynski @Shinigami92 @ST-DDT @xDivisionByZerox @damienwebdev @import-brain