Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-43103: Add ObservationInfo.can_see_sky property #73

Merged
merged 14 commits into from
Mar 28, 2024
Merged

Conversation

timj
Copy link
Member

@timj timj commented Mar 20, 2024

No description provided.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 98.71795% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.24%. Comparing base (a7b8da8) to head (9dc425e).

Files Patch % Lines
python/astro_metadata_translator/file_helpers.py 83.33% 1 Missing ⚠️
...astro_metadata_translator/translators/megaprime.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   79.70%   88.24%   +8.54%     
==========================================
  Files          36       37       +1     
  Lines        3148     3097      -51     
  Branches      672      658      -14     
==========================================
+ Hits         2509     2733     +224     
+ Misses        508      247     -261     
+ Partials      131      117      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timj timj force-pushed the tickets/DM-43103 branch from 95cda85 to 6f1f019 Compare March 26, 2024 16:16
timj added 6 commits March 26, 2024 14:11
This lets click override stdout for testing, which it can't
do if sys.stdout is forced to be the default.
Defaulting to sys.stderr hides it from click. Rather than
trying to use click.echo replace it with a logger (which
will default to using sys.stderr).
@timj timj force-pushed the tickets/DM-43103 branch from c520a2d to a0cb59b Compare March 27, 2024 18:07
@timj timj requested a review from erykoff March 27, 2024 18:26
Copy link

@erykoff erykoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, but I am wondering about the few error cases that coverage says aren't covered. Also, does this have any effect on the race condition that we see sometimes in the tests? (That may have been fixed elsewhere, I don't recall...)

@timj
Copy link
Member Author

timj commented Mar 28, 2024

I added 5% coverage so I'm going to call that a win.

This has no effect on the test failure we were getting. On a previous PR I made the test more expressive in its failure output so we would know why it is failing but I don't think it's failed since I added that.

@timj timj force-pushed the tickets/DM-43103 branch from a0cb59b to c1a5825 Compare March 28, 2024 00:33
This will stop it complaining about some lines that are never
going to be hit in a test.
@timj
Copy link
Member Author

timj commented Mar 28, 2024

Ok. I added extra coverage and stopped it complaining about some lines that were never going to be tested.

@timj timj merged commit 5b7d32c into main Mar 28, 2024
17 checks passed
@timj timj deleted the tickets/DM-43103 branch March 28, 2024 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants