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

Extract constant URLs to own module #591

Merged
merged 4 commits into from
Aug 30, 2024
Merged

Conversation

mfisher87
Copy link
Member

No description provided.

Copy link

github-actions bot commented Aug 27, 2024

Binder 👈 Launch a binder notebook on this branch for commit 2dc7e78

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 06564c1

Binder 👈 Launch a binder notebook on this branch for commit cbaff78

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.56%. Comparing base (bd09d73) to head (cbaff78).
Report is 1 commits behind head on development.

Files with missing lines Patch % Lines
icepyx/core/granules.py 25.00% 3 Missing ⚠️
icepyx/core/is2ref.py 66.66% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (bd09d73) and HEAD (cbaff78). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (bd09d73) HEAD (cbaff78)
3 2
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #591      +/-   ##
===============================================
- Coverage        71.02%   65.56%   -5.47%     
===============================================
  Files               36       37       +1     
  Lines             3037     3043       +6     
  Branches           594      594              
===============================================
- Hits              2157     1995     -162     
- Misses             769      963     +194     
+ Partials           111       85      -26     

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

@mfisher87 mfisher87 added the review::quick This change should not require more than 10 minutes to review label Aug 27, 2024
Comment on lines +8 to +10
ORDER_BASE_URL: Final = f"{EGI_BASE_URL}/request"

DOWNLOAD_BASE_URL: Final = "https://n5eil02u.ecs.nsidc.org/esir"
Copy link
Member

Choose a reason for hiding this comment

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

Having just been working on incorporating GEDI into icepyx, I'm noting that these urls are NSIDC specific even though the names are more generic. It's not a dealbreaker, but something we'll have to confront in the near future.

More generally, I didn't see any immediate analog in earthaccess, but moving towards directly using any urls that are easily surfaceable there is saving us work down the line.

@mfisher87
Copy link
Member Author

Hey @JessicaS11 now that we've merged #547, can you give this another review to trigger integration tests?

@mfisher87
Copy link
Member Author

Looks like something isn't right :) Looking in to it!

@JessicaS11
Copy link
Member

Looks like something isn't right :) Looking in to it!

I'm noticing that the checks ran (successfully, and interestingly included the integration test) after the precommit auto fixes (06564c1). My approval triggered the UML auto-update (but not the integration test?), but that commit only triggered readthedocs and precommit checks. I'm guessing the status is missing because for some reason the tests weren't triggered for the head commit, but I don't quite understand why.

@mfisher87
Copy link
Member Author

I especially don't understand why the unit tests weren't triggered!

@mfisher87 mfisher87 closed this Aug 29, 2024
@mfisher87 mfisher87 reopened this Aug 29, 2024
@JessicaS11
Copy link
Member

Maybe open PRs are just going to be a bit grumpy at us until we're into ones opened after the actions were implemented?

@mfisher87
Copy link
Member Author

Weird. Maybe you're right and all will be fine from here!

I opened #595 in case this continues to give us trouble. I want to avoid spending too much of our limited time getting things perfect :)

@mfisher87 mfisher87 merged commit 572e48c into development Aug 30, 2024
9 of 10 checks passed
@mfisher87 mfisher87 deleted the extract-urls-module branch August 30, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review::quick This change should not require more than 10 minutes to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants