Skip to content

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 22, 2023

Fix the incorrect defaults introduced in #2055

See also:


Alternative solution:

  • Change the documented defaults

Additional suggestion:

Deprecate the method entirely (because it is probably more useful if done like this by the user:

someMethod().replace(/(?<=.{4})\w(?=.{2})/g,'*')

which converts DE241358431563434532 to DE24**************32

We should probably document that somewhere in our docs too, although I assume they should already have something similar in their code somewhere.

@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent m: finance Something is referring to the finance module labels Oct 22, 2023
@ST-DDT ST-DDT added this to the v8.x milestone Oct 22, 2023
@ST-DDT ST-DDT requested review from a team October 22, 2023 13:57
@ST-DDT ST-DDT self-assigned this Oct 22, 2023
@codecov
Copy link

codecov bot commented Oct 22, 2023

Codecov Report

Merging #2494 (e62fb5b) into next (8542ef3) will increase coverage by 0.00%.
Report is 1 commits behind head on next.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2494   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files        2779     2779           
  Lines      249273   249273           
  Branches     1084     1084           
=======================================
+ Hits       248230   248232    +2     
+ Misses       1015     1013    -2     
  Partials       28       28           
Files Coverage Δ
src/modules/finance/index.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@xDivisionByZerox xDivisionByZerox added the s: accepted Accepted feature / Confirmed bug label Oct 22, 2023
@matthewmayer
Copy link
Contributor

Given that the defaults were accidentally changed during a method rename and we didn't catch that, suggests there should be a test for that? e.g. check that maskedNumber() with no params includes (... )

@ST-DDT ST-DDT marked this pull request as draft October 23, 2023 06:34
@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 26, 2023

Given that the defaults were accidentally changed during a method rename and we didn't catch that, suggests there should be a test for that? e.g. check that maskedNumber() with no params includes (... )

Done

@ST-DDT ST-DDT marked this pull request as ready for review October 26, 2023 19:36
@ST-DDT ST-DDT requested review from xDivisionByZerox and a team October 26, 2023 19:37
matthewmayer
matthewmayer previously approved these changes Oct 27, 2023
@ST-DDT ST-DDT dismissed stale reviews from xDivisionByZerox and matthewmayer via ac10f9a October 27, 2023 07:41
Co-authored-by: DivisionByZero <leyla.jaehnig@gmx.de>
@ST-DDT ST-DDT requested a review from matthewmayer October 27, 2023 07:41
@ST-DDT ST-DDT requested review from xDivisionByZerox and a team October 27, 2023 07:41
@ST-DDT ST-DDT requested review from xDivisionByZerox and a team October 29, 2023 15:23
@ST-DDT ST-DDT enabled auto-merge (squash) November 7, 2023 16:34
@ST-DDT ST-DDT merged commit e0ba50b into next Nov 7, 2023
@ST-DDT ST-DDT deleted the fix/finance/maskedNumber-defaults branch November 9, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working m: finance Something is referring to the finance module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants