docs: improving readability of UUID string

[Kcops11]

Updated comment to be more closely in line with semantic style guide.

#1466

[codecov]

Codecov Report

Merging #1622 (4a8f8fa) into next (99308d4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             next    #1622   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files        2346     2347    +1     
  Lines      235646   235657   +11     
  Branches     1142     1144    +2     
=======================================
+ Hits       234798   234809   +11     
  Misses        826      826           
  Partials       22       22           

Impacted Files	Coverage Δ
src/locales/pt_BR/person/index.ts	100.00% <100.00%> (ø)
src/locales/pt_BR/person/western_zodiac_sign.ts	100.00% <100.00%> (ø)
src/modules/string/index.ts	100.00% <100.00%> (ø)
src/modules/location/index.ts	98.70% <0.00%> (-0.22%)	⬇️
src/modules/internet/user-agent.ts	96.44% <0.00%> (+0.59%)	⬆️

[ST-DDT]

Sorry for the late response.
We looked over this, and sadly we feel that this is not what the issue is about.

The issue is about the actual implementation of the method using bit operations as described here:
#1466 (comment)
In this comment we opted for adding a comment to the implementation explaining what the nit magic actually does, so that a future reader knows exactly what happens.

As for the additions to the jsdocs.
Most of the added docs don't add relevant information for the user.
Except for the first sentence that it "is randomly generated".

Please revert the other documentation additions and add a comment to the bit operations in the implementation (don't refer to bits, only values/value ranges).

[Kcops11]

Okay, I will make changes and get back to you.

[Kcops11]

I have updated the code with a comment and removed most of the doc comment I previously added. Please let me know if you would like me to make any additional changes or if my changes do not reflect what you are looking for.

[Kcops11]

Hello @ST-DDT and @Shinigami92. I coming back to this after finishing my finals. Has a consensus been reached between you regarding the direction this should take?

It would seem as though this is it: #1622 (comment). Let me know!

[xDivisionByZerox]

Hello @ST-DDT and @Shinigami92. I coming back to this after finishing my finals. Has a consensus been reached between you regarding the direction this should take?

It would seem as though this is it: #1622 (comment). Let me know!

While I'm neither of them I think the approach from @ST-DDT is our best shot here.

• It improves readability (the main goal of this PR)
• has negligible performance influence
• allows us to activate the no-bitwise eslint rule in error mode

[Kcops11]

I have attempted to fix some of the formatting errors pointed out by Prettier. Ill try fixing any additional errors after I have successfully handled the formatting.

[xDivisionByZerox]

I have attempted to fix some of the formatting errors pointed out by Prettier. Ill try fixing any additional errors after I have successfully handled the formatting.

It seems like you are fixing those errors on your own...
To fix formatting errors simply run npm run format
To fix the failing snapshot test simply run npx vitest -u (-u updates the snapshots)

[matthewmayer]

I'm not sure this is that much readable than the original version.

I think a comment would be helpful, maybe:

Each x in the template is replaced with a random hex digit from `0` to `f`, and y is replaced with a random hex digit from the range `{8, 9, a, b}` only.

Also could consider

• Writing "11" as 0xb instead
• splitting replacePlaceholders into two seperate functions, one for replacing the x's and one for replacing the y's?

[ST-DDT]

@matthewmayer Like this? (Prettier gives me a hard time to format this in a readable way)

  uuid(): string {
    return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'
      .replace(/x/g, () => this.faker.number.hex({ min: 0x0, max: 0xf }))
      .replace(/y/g, () => this.faker.number.hex({ min: 0x8, max: 0xb }));
  }

I'm not sure about using the hex notation for min.

[matthewmayer]

Yeah that seems much more readable to me.

[matthewmayer]

Sometimes I think writing more code makes it easier to read.

Like, adding the explicit min and max for the x replacement isn't necessary but makes it easier to understand, because you don't need to go check what the default parameter values are for hex() to understand the line of code. And it also makes it easier to see the difference between that line and the line below.