deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

feat(bytes): `@std/[email protected]`

Open iuioiua opened this issue 9 months ago • 4 comments

Reviews, please thoroughly review the source code and the documentation of this package and approve once:

  1. You foresee no breaking changes.
  2. Documentation and implementations are satisfactory, make sense, and don't raise concerns.

This is our first time stabilizing a package, so please point out any stones we left unturned in the process.

Closes #4629

iuioiua avatar Apr 29 '24 03:04 iuioiua

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.36%. Comparing base (3155f00) to head (eb02236). Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4651   +/-   ##
=======================================
  Coverage   91.36%   91.36%           
=======================================
  Files         477      477           
  Lines       37334    37334           
  Branches     5325     5325           
=======================================
  Hits        34109    34109           
  Misses       3164     3164           
  Partials       61       61           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 29 '24 03:04 codecov[bot]

One thing I noticed is that the 3rd params of copy, includesNeedle, indexOfNeedle, and lastIndexOfNeedle don't exactly follow the style guide rule (Exported functions: max 2 args, put the rest into an options object.).

They all use optional primitive type (offset?: number) as the 3rd param consistently. I personally think this is intentionally in this way, and we don't need to change them (instead we probably should update the style guide)

What do you think?

kt3k avatar May 02 '24 05:05 kt3k

Yeah, that's a great idea. I think options objects suite functions/methods with broader functionality and the potential to expand their functionality in the future. Non-object options suite functions/methods with narrower functionality and are unlikely to expand their functionality in the future. Perhaps this distinction should be noted.

iuioiua avatar May 02 '24 06:05 iuioiua

Yes I agree. I don't think we should make them objects. Let's update the style guide

lucacasonato avatar May 02 '24 08:05 lucacasonato