ENH: Add conditional to adapt behaviour depending on astropy version and warn user

[vandalt]

This is a follow-up for #60 by @neutrinoceros. As discussed there, I added a warning that commentary cards are removed. I also changed working version to 5.0rc1, the first release candidate with the upstream fix included. I also added tests to 1) check that the card is included in 5.0 and 2) Check that the warning is issued before 5.0.

[codecov]

Codecov Report

Merging #73 (1e6d30c) into master (ccd3c68) will increase coverage by 0.32%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   47.88%   48.21%   +0.32%     
==========================================
  Files          19       19              
  Lines        3621     3646      +25     
==========================================
+ Hits         1734     1758      +24     
- Misses       1887     1888       +1     

Flag	Coverage Δ
unittests	48.21% <97.14%> (+0.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
amical/tests/test_extraction.py	97.43% <96.15%> (-2.57%)	⬇️
amical/mf_pipeline/bispect.py	76.28% <100.00%> (+0.23%)	⬆️

---

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 ccd3c68...1e6d30c. Read the comment docs.

[neutrinoceros]

Looks good, thank you for continuing this work !
I made a couple suggestions to improve the tests, but in general I'm in favour of this simply superseding my own PR, so I'll close #60

[neutrinoceros]

Another important thing is that we should test this properly using the pre release. I'll open a PR to do just that

[vandalt]

Thanks @neutrinoceros ! For the first suggestion, I kept the two tests for now. I'm not opposed to removing the "drops" one. I have little experience with unit tests, so I'd let you decide what to do here. My initial idea was to test the behaviour because there is a warning related to it, so if the behaviour is fixed, a failing test would flag that the warning needs to be removed.

[neutrinoceros]

My initial idea was to test the behaviour because there is a warning related to it, so if the behaviour is fixed, a failing test would flag that the warning needs to be removed.

It's a good idea. In general, it's good to test failing cases, not just successes. Another rule of thumb is to avoid testing implementation details instead of behaviour, because it tends to rigidify the design, and it's a burden to maintain.

So ideally here we should have:

• a test that checks that the warning is raised if astropy < 5.0 (using @pytest.mark.skipif)
• another test, identical to the one we dedicate to astropy 5.0, but reserved for older versions (using @pytest.mark.skipif again) AND decorated with @pytest.mark.xfail (see https://docs.pytest.org/en/6.2.x/skipping.html#xfail-mark-test-functions-as-expected-to-fail)

[neutrinoceros]

we're getting there, thank you for your patience @vandalt !

[vandalt]

Thanks for your detailed comments @neutrinoceros ! I think I addressed all of them, but let me know if I forgot something or if there are others.

EDIT: Typo

[neutrinoceros]

I think this should be my final round of review. I apologies for misleading you into combining xfail and skipif, I realised while reading this that we should probably never have to use more than one.

[neutrinoceros]

It'd be useful to run the "bleeding edge" job I just merged into master here, to check that your tests behave correctly with astropy's dev version.
Basically you'd need to rebase against the current state of the master branch and run the job manually on your fork. If you need help for that don't hesitate to ask

[vandalt]

I rebased on master and also rebased the master of my fork on master. I'm not sure how to run the "bleeding edge" CI though. I did run pytest in a virtualenv with the 5.0rc1 version of astropy and all tests passed.

[neutrinoceros]

On your fork, go to the "Actions" section, select the job as shown, and click the "run workflow" you can see on the right (select the branch attached to this PR)

[vandalt]

Thanks, I had just figured out how to launch it, so it's running. However, all previous CI runs on my fork failed with the following error in the "Upload coverage to codecov" section:

{'detail': ErrorDetail(string='Could not find a repository, try using repo upload token', code='not_found')}

Does it mean I need to setup codecov on the fork ?

[neutrinoceros]

No you don't. Actually you could deactivate actions on your fork in general, it's not necessary.

[neutrinoceros]

so the job is here https://github.com/vandalt/AMICAL/runs/4094406054?check_suite_focus=true
it WILL fail because of Codecov, I just want to make sure it doesn't fail for a different reason

[vandalt]

Indeed it did, but the tests worked as expected I think.

Or at least it gives the same result as my local virtualenv with astropy 5.

[neutrinoceros]

I confirm that the results are consistent with running locally with the release candidate. Hurray !
Now for one ultimate, and probably quick, round

[neutrinoceros]

I do not understand why Codecov reports this as not hit. It runs on my machine whatever version of astropy I use. Any clue ? (not needed to get this merged)

[vandalt]

No sorry... Works on my machine as well, and I don't know much about Codecov

[neutrinoceros]

Alright, thanks again for your patience, I think this is mergeable now. I'll give you the opportunity to respond to my last suggestions and after that I'll push the button :-)

[vandalt]

Thanks for the help on this @neutrinoceros. Sorry I was away today, so your last suggestion was accepted via mobile and the commit message is not very informative.