fix: change random.word to be based on definitions.word.* #276

pkuczynski · 2022-01-23T21:45:04Z

Fixes #113
Fixes #218

prisis · 2022-01-23T23:24:10Z

I would put into 6.1 @import-brain

ejcheng · 2022-01-24T00:55:18Z

I would put into 6.1 @import-brain

Done

pkuczynski · 2022-01-24T11:25:07Z

How about the review @prisis ?

prisis · 2022-01-24T11:40:26Z

@pkuczynski hope it's fine with you if i wait with the review a bit.

I would like to fix the tests more and remove all the mock function call, so we now that we break something.

And this is a runtime change of the code, so we will not merge it into v6.0 for now. But im thankful for you spend time on this PR.

Note: ignore the approve, clicked the wrong button...

prisis · 2022-01-24T11:41:25Z

src/random.ts

@@ -159,53 +159,28 @@ export class Random {
    return this.faker.datatype.boolean();
  }

-  // TODO: have ability to return specific type of word? As in: noun, adjective, verb, etc


The question is if we want to have this feature too.

We already have. Users can use faker.word.xyz to do it... That's why I removed this comment.

pkuczynski · 2022-01-24T12:13:58Z

The problem is, that the longer you wait with merging, the higher chance of conflict. I worked on this as it was listed as "help needed". This feature does not have any tests, so not sure what mocks do you want to remove?

Shinigami92 · 2022-01-24T12:25:52Z

The problem is, that the longer you wait with merging, the higher chance of conflict. I worked on this as it was listed as "help needed". This feature does not have any tests, so not sure what mocks do you want to remove?

Sorry but this is irrelevant to the current situation of the project.

We have a roadmap https://github.com/orgs/faker-js/projects/2 with the milestone https://github.com/faker-js/faker/milestone/1
We are thanksful that you try to contribute, but if you want to prevent merge conflicts right now, than wait until we release 6.0.0 and stop opening PR for now

This said, I can somewhat promise you that src/random.ts will not change that much anymore. The project stability is more regarding to the test-suite, bundling-setup and documentation. Multiple times said: no runtime changes before 6.0.0 released

src/random.ts

pkuczynski · 2022-01-28T13:52:04Z

Possibly in the future we might consider to move this to faker.word.any() as this what happened with other methods from this module...

What do you think? I could record this idea as a GitHub issue for future consideration...

griest024 · 2022-01-28T14:01:02Z

Possibly in the future we might consider to move this to faker.word.any() as this what happened with other methods from this module...

What do you think? I could record this idea as a GitHub issue for future consideration...

I think that is reasonable.

pkuczynski · 2022-01-28T20:16:42Z

I think that is reasonable.

Here you go #339 :)

Shinigami92 · 2022-03-21T12:51:23Z

As this is changing runtime behavior and we already want to do #339 anyways, I will close this PR in favor of #339

pkuczynski · 2022-03-21T13:00:57Z

@Shinigami92 I think this should be still merged, because it contains valid changes. Then the move in #339 can happen more easily...

# Conflicts: # src/random.ts

Shinigami92 · 2022-03-21T13:05:38Z

@ST-DDT and I decided this together via screen-share some minutes ago
We do not see to merge this because it just changes behavior that we already want to deprecate anyways.
What you could do right now would be to just add a console.warn that logs that this function will be deprecated so we can already deprecate it in v6.1.

Let me also say that I do totally not expect that the move from initial to v6.0.0 will take same amount of time as from v6.0 to v6.1 🙂
We want now to move on a bit faster

pkuczynski · 2022-03-21T13:11:02Z

I mean this function after the changes done currently, can be with pretty much (almost) no change moved to word.any and random.word deprecated. It does slightly change the behaviour, that's true but not in a way which is dangerous in any way and it already does fix the issues reported in #113... So I still see this as perfectly mergeable...

pkuczynski · 2022-03-21T15:03:00Z

@Shinigami92 @ST-DDT rebased and tests fixed

Shinigami92

I'm more and more tempted to not accept this PR 😕 Sorry...

I re-read through #113 and I thing we should go with the suggested proposal to just regex them out if they generate "invalid" words and then make a retry

With this PR (#276) we loose many possible words, especially hacker words, were you can see that they are not in generated system filenames anymore

I will block this from my side with a request changes, but if another maintainer is on my side, then I would vote for close this PR

pkuczynski · 2022-03-22T08:51:59Z

Well, at least we get more sensible words with this pr comparing to before... but yeah, do what you wish...

ST-DDT · 2022-03-22T10:15:42Z

Well, at least we get more sensible words with this pr comparing to before... but yeah, do what you wish...

What do you consider a "sensible" word? IMO we have to decide first what we expect from this method.
AFAIK this method is used in file name generation.
Should those contain Chinese letters/words in Chinese locales?
What about the opposite? Should it partially return English file names in Chinese locales

pkuczynski · 2022-03-22T14:07:38Z

Well, at least we get more sensible words with this pr comparing to before... but yeah, do what you wish...

What do you consider a "sensible" word? IMO we have to decide first what we expect from this method. AFAIK this method is used in file name generation. Should those contain Chinese letters/words in Chinese locales? What about the opposite? Should it partially return English file names in Chinese locales

I wold consider noun, adjective, verb, ... a sensible word. While things like streetSuffix or currencyName does not seem something I would expect from random.word (or word.any in the future). For example would as long as returned at the moment could be considered as a word? Even if you split it into as and long. The second one seems ok, but as? Don't think so...

Looking at the wikipedia https://en.wikipedia.org/wiki/Word:

In linguistics, a word of a spoken language can be defined as the smallest sequence of phonemes that can be uttered in isolation with objective or practical meaning. In many languages, words also correspond to sequences of graphemes ("letters") in their standard writing systems that are delimited by spaces wider than the normal inter-letter space, or by other graphical conventions.[1] The concept of "word" is usually distinguished from that of a morpheme, which is the smallest unit of word which has a meaning, even if it will not stand on its own together or in other small words.

pkuczynski · 2022-03-22T14:12:38Z

For filename generation we could also use lorem.slug().

In other languages, I would expect the filename is generated in the selected locale, unless there is no translation for it. Wouldn't you?

pkuczynski · 2022-03-24T15:27:53Z

I am not doing anything with his PR until you make a decision, right?

Shinigami92 · 2022-03-25T08:31:09Z

In yesterdays maintainers-team-meeting we discussed that we want to have the random.word function just as it is right now, but add a @deprecated JSDoc comment to it and a warning for removal
We want to have the word.any function directly and it should be implemented like you are already doing in this PR

pkuczynski · 2022-03-25T12:02:33Z

OK!

fix: change random.word to be based on definitions.word.*

c9e993a

pkuczynski requested a review from a team as a code owner January 23, 2022 21:45

ejcheng added the p: 1-normal Nothing urgent label Jan 23, 2022

ejcheng added this to the v6.0.0 - Project stability milestone Jan 23, 2022

pkuczynski mentioned this pull request Jan 23, 2022

random.word() can return undesirable non-alpha characters #113

Closed

ejcheng modified the milestones: v6.0.0 - Project stability, v6.1.0 - First new features Jan 24, 2022

Shinigami92 assigned pkuczynski Jan 24, 2022

prisis previously approved these changes Jan 24, 2022

View reviewed changes

prisis self-requested a review January 24, 2022 11:42

griest024 self-requested a review January 24, 2022 17:44

griest024 suggested changes Jan 24, 2022

View reviewed changes

src/random.ts Outdated Show resolved Hide resolved

griest024 mentioned this pull request Jan 24, 2022

locale(en): remove word examples that contain spaces #281

Merged

Update random.ts

5b5c566

pkuczynski dismissed prisis’s stale review via 5b5c566 January 24, 2022 20:20

Update random.ts

6087ada

pkuczynski requested a review from griest024 January 24, 2022 20:26

griest024 previously approved these changes Jan 25, 2022

View reviewed changes

Shinigami92 mentioned this pull request Jan 28, 2022

Use word methods in random.word() #129

Closed

ST-DDT previously approved these changes Jan 28, 2022

View reviewed changes

pkuczynski mentioned this pull request Jan 28, 2022

Move random.word to word.any #339

Closed

Shinigami92 closed this Mar 21, 2022

pkuczynski reopened this Mar 21, 2022

pkuczynski added 2 commits March 21, 2022 14:04

Merge branch 'main' into random-word

2dd3a1d

# Conflicts: # src/random.ts

Merge branch 'main' into random-word

4a88034

# Conflicts: # src/random.ts

Merge remote-tracking branch 'origin/random-word' into random-word

a1e24a7

pkuczynski dismissed stale reviews from ST-DDT and griest024 via a1e24a7 March 21, 2022 13:08

fix tests

84eac47

pkuczynski added 2 commits March 21, 2022 16:03

Merge branch 'main' into random-word

7d38865

fix test

e377f18

Shinigami92 marked this pull request as draft March 21, 2022 17:05

Merge branch 'main' into random-word

3cc73ef

Shinigami92 requested changes Mar 22, 2022

View reviewed changes

ST-DDT added the s: needs decision Needs team/maintainer decision label Mar 22, 2022

Shinigami92 removed the s: needs decision Needs team/maintainer decision label Mar 25, 2022

pkuczynski mentioned this pull request Mar 28, 2022

feat(word): add sample method #714

Merged

pkuczynski closed this Mar 28, 2022

pkuczynski deleted the random-word branch March 28, 2022 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change random.word to be based on definitions.word.* #276

fix: change random.word to be based on definitions.word.* #276

pkuczynski commented Jan 23, 2022 •

edited

Loading

prisis commented Jan 23, 2022

ejcheng commented Jan 24, 2022

pkuczynski commented Jan 24, 2022

prisis commented Jan 24, 2022 •

edited

Loading

prisis Jan 24, 2022

pkuczynski Jan 24, 2022

pkuczynski commented Jan 24, 2022

Shinigami92 commented Jan 24, 2022

pkuczynski commented Jan 28, 2022 •

edited

Loading

griest024 commented Jan 28, 2022

pkuczynski commented Jan 28, 2022

Shinigami92 commented Mar 21, 2022

pkuczynski commented Mar 21, 2022

Shinigami92 commented Mar 21, 2022

pkuczynski commented Mar 21, 2022

pkuczynski commented Mar 21, 2022

Shinigami92 left a comment

pkuczynski commented Mar 22, 2022

ST-DDT commented Mar 22, 2022 •

edited

Loading

pkuczynski commented Mar 22, 2022

pkuczynski commented Mar 22, 2022

pkuczynski commented Mar 24, 2022

Shinigami92 commented Mar 25, 2022

pkuczynski commented Mar 25, 2022

fix: change random.word to be based on definitions.word.* #276

fix: change random.word to be based on definitions.word.* #276

Conversation

pkuczynski commented Jan 23, 2022 • edited Loading

prisis commented Jan 23, 2022

ejcheng commented Jan 24, 2022

pkuczynski commented Jan 24, 2022

prisis commented Jan 24, 2022 • edited Loading

prisis Jan 24, 2022

Choose a reason for hiding this comment

pkuczynski Jan 24, 2022

Choose a reason for hiding this comment

pkuczynski commented Jan 24, 2022

Shinigami92 commented Jan 24, 2022

pkuczynski commented Jan 28, 2022 • edited Loading

griest024 commented Jan 28, 2022

pkuczynski commented Jan 28, 2022

Shinigami92 commented Mar 21, 2022

pkuczynski commented Mar 21, 2022

Shinigami92 commented Mar 21, 2022

pkuczynski commented Mar 21, 2022

pkuczynski commented Mar 21, 2022

Shinigami92 left a comment

Choose a reason for hiding this comment

pkuczynski commented Mar 22, 2022

ST-DDT commented Mar 22, 2022 • edited Loading

pkuczynski commented Mar 22, 2022

pkuczynski commented Mar 22, 2022

pkuczynski commented Mar 24, 2022

Shinigami92 commented Mar 25, 2022

pkuczynski commented Mar 25, 2022

pkuczynski commented Jan 23, 2022 •

edited

Loading

prisis commented Jan 24, 2022 •

edited

Loading

pkuczynski commented Jan 28, 2022 •

edited

Loading

ST-DDT commented Mar 22, 2022 •

edited

Loading