Skip to content

Refactor umm dict accessors - #1073

Merged
jhkennedy merged 3 commits into
earthaccess-dev:mainfrom
jhkennedy:dict-refactor
Aug 25, 2025
Merged

Refactor umm dict accessors#1073
jhkennedy merged 3 commits into
earthaccess-dev:mainfrom
jhkennedy:dict-refactor

Conversation

@jhkennedy

@jhkennedy jhkennedy commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

This is a small PR to refactor dict access so that instad of doing an upfront check for key existance and then accessing it, we use the built in .get method to handle this.

I don't see any value in adding such a minor refactor to the changelog, so I have not updated it, but would be happy to if people feel otherswise.

Pull Request (PR) draft checklist - click to expand
  • Please review our
    contributing documentation
    before getting started.
  • Populate a descriptive title. For example, instead of "Updated README.md", use a
    title such as "Add testing details to the contributor section of the README".
    Example PRs: #763
  • Populate the body of the pull request with:
  • Update CHANGELOG.md with details about your change in a section titled
    ## Unreleased. If such a section does not exist, please create one. Follow
    Common Changelog for your additions.
    Example PRs: #763
  • Update the documentation and/or the README.md with details of changes to the
    earthaccess interface, if any. Consider new environment variables, function names,
    decorators, etc.

Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!

Pull Request (PR) merge checklist - click to expand

Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the @nsidc/earthaccess-support team in a comment and we
will help you out!

  • Add unit tests for any new features.
  • Apply formatting and linting autofixes. You can add a GitHub comment in this Pull
    Request containing "pre-commit.ci autofix" to automate this.
  • Ensure all automated PR checks (seen at the bottom of the "conversation" tab) pass.
  • Get at least one approving review.

@github-actions

github-actions Bot commented Aug 21, 2025

Copy link
Copy Markdown

Binder 👈 Launch a binder notebook on this branch for commit 00c8aad

I will automatically update this comment whenever this PR is modified

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

Binder 👈 Launch a binder notebook on this branch for commit 5ba8c33

@jhkennedy
jhkennedy marked this pull request as ready for review August 22, 2025 17:44
betolink
betolink previously approved these changes Aug 22, 2025

@betolink betolink left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good to me!

@owenlittlejohns owenlittlejohns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I love this clean-up! I'm a big fan of dict.get('key') over dict['key'].

I had two quick comments, but they are kind of out of scope just from me skimming the rest of the file. The landing_page one is likely not an improvement, so definitely feel free to ignore that one.

Comment thread earthaccess/results.py
return ""
return self["umm"].get("Abstract", "")

def landing_page(self) -> str:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it worth doing something with landing_page, too? It'll need to be a bit sneakier where there's a list involved, but maybe something like:

def landing_page(self) -> str:
    return next(iter(collection_results[0]._filter_related_links("LANDING PAGE")), "")

That might not be as readable, though, so feel free to ignore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is a little harder to reason about the fallback value, at least for me.

I think the most readable form might actually be:

return links[0] if len(links) > 0 else ""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got to love a good ternary. That works for me.

Comment thread earthaccess/results.py

@owenlittlejohns owenlittlejohns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good!

Comment thread earthaccess/results.py
return ""
return self["umm"].get("Abstract", "")

def landing_page(self) -> str:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got to love a good ternary. That works for me.

Comment thread earthaccess/results.py

@mfisher87 mfisher87 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Nice refactor ⭐

@jhkennedy
jhkennedy merged commit e2f4bae into earthaccess-dev:main Aug 25, 2025
11 checks passed
@jhkennedy
jhkennedy deleted the dict-refactor branch August 25, 2025 22:44
@github-project-automation github-project-automation Bot moved this to ✅ Done in earthaccess Mar 3, 2026
@mfisher87 mfisher87 removed this from earthaccess Mar 3, 2026
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.

4 participants