search

oda-hub / dispatcher-plugin-integral

Other
1 stars 1 forks source link

init OSAVersion #63

Closed burnout87 closed 3 years ago

burnout87 commented 3 years ago

Initialized as a Name parameter

pep8speaks commented 3 years ago

Hello @burnout87! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 131:80: E501 line too long (92 > 79 characters) Line 132:80: E501 line too long (86 > 79 characters) Line 133:80: E501 line too long (98 > 79 characters) Line 154:80: E501 line too long (88 > 79 characters) Line 155:80: E501 line too long (80 > 79 characters) Line 166:80: E501 line too long (92 > 79 characters) Line 167:80: E501 line too long (86 > 79 characters) Line 168:80: E501 line too long (98 > 79 characters) Line 172:80: E501 line too long (103 > 79 characters)

Comment last updated at 2021-10-05 09:30:12 UTC
burnout87 commented 3 years ago

This only tackles the initialization of a OSAVersion parameter. An additional change would ideally be required at dispatcher level.

burnout87 commented 3 years ago

This only tackles the initialization of a OSAVersion parameter. An additional change would ideally be required at dispatcher level.

Just noticed that also for the time parameters (eg T1 and T2) units is not set, and it looks like it should be the related format, also this, should be tacked at a dispatcher-app level.

codecov-commenter commented 3 years ago

Codecov Report

Merging #63 (944314f) into master (8ed14e3) will increase coverage by 0.07%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   24.96%   25.03%   +0.07%     
==========================================
  Files          11       11              
  Lines        1374     1370       -4     
==========================================
  Hits          343      343              
+ Misses       1031     1027       -4
Impacted Files Coverage Δ
cdci_osa_plugin/osa_common_pars.py 47.91% <0.00%> (+1.91%) :arrow_up:
cdci_osa_plugin/osa_isgri.py 38.46% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8ed14e3...944314f. Read the comment docs.

volodymyrss commented 3 years ago

the tests for ISGRI and JEMX are almost identical. Could they be combined with parametrization?

burnout87 commented 3 years ago

the tests for ISGRI and JEMX are almost identical. Could they be combined with parametrization?

I thought the same, but I went for this approach since we already had two instrument specific test modules.

volodymyrss commented 3 years ago

the tests for ISGRI and JEMX are almost identical. Could they be combined with parametrization?

I thought the same, but I went for this approach since we already had two instrument specific test modules.

We also have test groups not specific to instruments.

Why further increase duplication, even if there is some already. For whatever reason it was done before. It's easier to maintain code without duplication, in case of changes it's not necessary to duplicate them too.

I do not suppose it's any more difficult to do it in two tests instead of four? Well, if it's not convenient now, we can leave a note and do it later.

burnout87 commented 3 years ago

the tests for ISGRI and JEMX are almost identical. Could they be combined with parametrization?

I thought the same, but I went for this approach since we already had two instrument specific test modules.

We also have test groups not specific to instruments.

Why further increase duplication, even if there is some already. For whatever reason it was done before. It's easier to maintain code without duplication, in case of changes it's not necessary to duplicate them too.

I do not suppose it's any more difficult to do it in two tests instead of four? Well, if it's not convenient now, we can leave a note and do it later.

and now they are