search

mljs / spectra-processing

Various methods to help spectra processing
https://mljs.github.io/spectra-processing/
MIT License
7 stars 12 forks source link

sequentialFill #221

Closed lpatiny closed 7 months ago

lpatiny commented 7 months ago
codecov[bot] commented 7 months ago

Codecov Report

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

Project coverage is 96.32%. Comparing base (2f4de0c) to head (ce289d3). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #221 +/- ## ========================================== + Coverage 96.22% 96.32% +0.09% ========================================== Files 174 176 +2 Lines 3418 3397 -21 Branches 806 780 -26 ========================================== - Hits 3289 3272 -17 + Misses 127 123 -4 Partials 2 2 ```

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

lpatiny commented 7 months ago

@targos

There are 2 issues in this PR.

  1. I don't know if we should use 3 arguments + options or if you prefer {from, to, size}. I have no special preferences and I don't know what is the easiest to use.
  2. The return types are not ok

https://github.com/mljs/spectra-processing/blob/76f5e673d8dbb7ff1a1b0d366441df3e4fc8cfb6/src/x/__tests__/xSequentialFillFromStep.test.ts#L16-L18

If I try to specify as constructor an 'Array' I can push in the resulting array but TS would still pretend I'm not allowed to put because it is a Float64Array

We should have an object so that the code can be read without ambiguity.

targos commented 7 months ago
  1. Use an object to make it more readable
  2. The problem was a mistake in the return type. I pushed a fix with some other refactors.