Quasars / orange-spectroscopy

Other
51 stars 59 forks source link

[FIX] OWTileFile warn on preprocessor change #647

Closed stuart-cls closed 1 year ago

stuart-cls commented 1 year ago

Following #638

[Edit: Left None bug for another day, see #648]

I found a bug where the code that runs the preprocessor() wasn't checking for PreprocessorLists with None in them (or empty lists), so PreprocessorList tries to call None(). But I kept thinking of more weird edge cases from the multiple inputs, so I decided instead to just not add None in the first place (which is cleaner, I think).

But now when you have a None connected (empty Preprocess Spectra), and then change it to a preprocessor (Add an editor to the Preprocess Spectra), it crashes because the index state from the MultiInput doesn't match the PreprocessorList length.

@markotoplak Before I go any further down this rabbit hole, any thoughts?

codecov-commenter commented 1 year ago

Codecov Report

Merging #647 (0e4e2e4) into master (d497b3f) will increase coverage by 0.02%. The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
+ Coverage   89.32%   89.34%   +0.02%     
==========================================
  Files          71       71              
  Lines       11830    11836       +6     
==========================================
+ Hits        10567    10575       +8     
+ Misses       1263     1261       -2     
markotoplak commented 1 year ago

Hmm, to me it seems dangerous to ignore Nones withing handling inputs. I do not have an informed-enough idea what is happening in the background to argue it. Maybe that would make a problem if doing insert/set with or without nones?

Anyway, I'd rather clean this up later.

stuart-cls commented 1 year ago

So to me it seems there are two options:

  1. fix code all the way down (easiest in PreprocessorList itself, I think) to handle having None in the list
  2. Make sure we never add None to the PreprocessorList (current status)

My first attempt was 1 but I ran into not wanting to edit PreprocessorList and started doing some unnecessarily complex workarounds.

I don't care which way but the status quo and this PR currently both crash on normal manipulations of the input.

stuart-cls commented 1 year ago

@markotoplak I guess the status quo crashes fairly quietly in a way that is easy to recover, just add a preprocessor. So I'll split out the warning stuff and leave the None for another day as you suggested