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: allow banned as string #819

Merged
merged 2 commits into from
May 22, 2022
Merged

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 8, 2022

Following PRs need to be merged before this PR:


I would love to even type the non-array version, but sadly we still have not RegexTypes (microsoft/TypeScript#41160) in TS and we are limited (https://stackoverflow.com/questions/66294091/how-to-create-a-type-that-describes-string-with-only-digits)

@Shinigami92 Shinigami92 added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Apr 8, 2022
@Shinigami92 Shinigami92 added this to the v6.2 - New small features milestone Apr 8, 2022
@Shinigami92 Shinigami92 self-assigned this Apr 8, 2022
@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label Apr 8, 2022
@Shinigami92 Shinigami92 mentioned this pull request Apr 8, 2022
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #819 (323ed59) into main (58145dd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #819   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files        1988     1988           
  Lines      210192   210279   +87     
  Branches      904      909    +5     
=======================================
+ Hits       209468   209555   +87     
  Misses        705      705           
  Partials       19       19           
Impacted Files Coverage Δ
src/modules/random/index.ts 98.14% <100.00%> (+0.46%) ⬆️

@Shinigami92 Shinigami92 added the needs test More tests are needed label Apr 8, 2022
@Shinigami92 Shinigami92 force-pushed the feat-allow-string-for-random-banned branch from 08095f0 to 4e42a65 Compare April 19, 2022 18:15
@Shinigami92 Shinigami92 removed the s: on hold Blocked by something or frozen to avoid conflicts label Apr 24, 2022
@Shinigami92 Shinigami92 force-pushed the feat-allow-string-for-random-banned branch from 54ec8aa to 402b3dc Compare April 24, 2022 19:40
@Shinigami92 Shinigami92 removed the needs test More tests are needed label Apr 24, 2022
@Shinigami92 Shinigami92 marked this pull request as ready for review April 24, 2022 19:41
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 24, 2022 19:41
import-brain
import-brain previously approved these changes Apr 24, 2022
@import-brain import-brain requested a review from a team April 24, 2022 22:07
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Apr 25, 2022
@ST-DDT ST-DDT requested a review from a team April 25, 2022 08:10
@xDivisionByZerox
Copy link
Member

What would be the use case of providing a string instead of an array? This would allow me to provide an option like bannedChars: 'aaaaaaaaaaaaaaaaaaaaaaaaaaA' where I would expect only this combination of charters to be baned.

@Shinigami92
Copy link
Member Author

What would be the use case of providing a string instead of an array? This would allow me to provide an option like bannedChars: 'aaaaaaaaaaaaaaaaaaaaaaaaaaA' where I would expect only this combination of charters to be baned.

Yes exactly, it's the same as bannedChars: 'aaaaaaaaaaaaaaaaaaaaaaaaaaA'.split(''), just a better UX

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Apr 25, 2022
@xDivisionByZerox
Copy link
Member

What would be the use case of providing a string instead of an array? This would allow me to provide an option like bannedChars: 'aaaaaaaaaaaaaaaaaaaaaaaaaaA' where I would expect only this combination of charters to be baned.

Yes exactly, it's the same as bannedChars: 'aaaaaaaaaaaaaaaaaaaaaaaaaaA'.split(''), just a better UX

Ok, I think you misunderstood me. By providing a string at the bannedChars option I would expect the exact char pattern to be ignored. Even if the property has "*Chars" in the name. Example:

// this is fine
const result = faker.datatype.string({
    bannedChars: 'abcd',
    count: 4,
});
console.log(result); // abcz

// this is not fine
const result = faker.datatype.string({
    bannedChars: 'abcd',
    count: 4,
});
console.log(result); // abcd

I'm sorry if this is confusing.

@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label May 5, 2022
@Shinigami92 Shinigami92 removed the s: awaiting more info Additional information are requested label May 7, 2022
@Shinigami92 Shinigami92 requested a review from ST-DDT May 7, 2022 15:05
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.

This fails the following tests:

it('should be able to ban some characters via string', () => {
  const actual = faker.random.alpha({
    count: 5,
    bannedChars: [
      'B',
      'C',
      'D',
      'E',
      'F',
      'G',
      'H',
      'I',
      'J',
      'K',
      'L',
      'M',
      'N',
      'O',
      'P',
      'Q',
      'R',
      'S',
      'T',
      'U',
      'V',
      'W',
      'X',
      'Y',
      'Z',
    ],
    upcase: true,
  });

  expect(actual).toHaveLength(5);
  expect(actual).toMatch(/^A{5}$/);
});

I know this isn't directly caused by this PR, but this PR explicitly lists uppercase characters as allowed (to be banned).

@Shinigami92
Copy link
Member Author

@ST-DDT You mean we need to add them to line 248 to 275?

@ST-DDT
Copy link
Member

ST-DDT commented May 7, 2022

I would like have a case: upper|lower|mixed parameter (mixed being the default).

@Shinigami92
Copy link
Member Author

I would like have a case: upper|lower|mixed parameter (mixed being the default).

okay, then this will be definitely another PR/issue

@ST-DDT
Copy link
Member

ST-DDT commented May 7, 2022

I would like have a case: upper|lower|mixed parameter (mixed being the default).

okay, then this will be definitely another PR/issue

I consider, this "fixed", if you remove the uppercase characters from the input (for now).
(and edit the jsdocs accordingly)

I think we should do that as part of the random -> strings switch.

@Shinigami92 Shinigami92 requested a review from ST-DDT May 7, 2022 16:21
@ST-DDT
Copy link
Member

ST-DDT commented May 7, 2022

Could you add a hint to the jsdocs regarding that behavior?

@Shinigami92 Shinigami92 force-pushed the feat-allow-string-for-random-banned branch from a0b93d9 to a62a43d Compare May 18, 2022 17:17
@Shinigami92
Copy link
Member Author

Await merge of #955

@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label May 18, 2022
@Shinigami92 Shinigami92 marked this pull request as draft May 18, 2022 17:18
@pkuczynski
Copy link
Member

@Shinigami92 can you remind me when did I request this? :) I tried to click around for a while and dust off my memory, but don't recon this request? :(

#797 (comment)

Ah yes, sorry! :) Indeed I requested this :) Then I think I had only this comment #819 (comment) which seems to be addressed?

pkuczynski
pkuczynski previously approved these changes May 18, 2022
@Shinigami92 Shinigami92 removed the s: on hold Blocked by something or frozen to avoid conflicts label May 21, 2022
@Shinigami92 Shinigami92 force-pushed the feat-allow-string-for-random-banned branch from 1b487e5 to 7b31e07 Compare May 21, 2022 18:26
@Shinigami92 Shinigami92 marked this pull request as ready for review May 21, 2022 18:27
@ST-DDT ST-DDT requested review from a team May 21, 2022 20:33
@Shinigami92 Shinigami92 merged commit a0d25bb into main May 22, 2022
@Shinigami92 Shinigami92 deleted the feat-allow-string-for-random-banned branch May 22, 2022 10:49
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants