Skip to content

Comments

extract filename from pyfive S3 Dataset correctly#197

Merged
valeriupredoi merged 6 commits intomainfrom
extract_filename_from_Dataset
Feb 20, 2026
Merged

extract filename from pyfive S3 Dataset correctly#197
valeriupredoi merged 6 commits intomainfrom
extract_filename_from_Dataset

Conversation

@valeriupredoi
Copy link
Collaborator

@valeriupredoi valeriupredoi commented Feb 16, 2026

Description

Dataset.id._filename currently returns None for an S3 Dataset (which is a pyfive Dataset)
Closes #199

Before you get started

Checklist

  • This pull request has a descriptive title and labels
  • This pull request has a minimal description (most was discussed in the issue, but a two-liner description is still desirable)
  • Unit tests have been added (if codecov test fails)
  • Any changed dependencies have been added or removed correctly (if need be)
  • If you are working on the documentation, please ensure the current build passes
  • All tests pass

@valeriupredoi valeriupredoi marked this pull request as draft February 17, 2026 11:33
@valeriupredoi
Copy link
Collaborator Author

valeriupredoi commented Feb 17, 2026

converting this to Draft for now, I am not seeing anything wrong on the GHA machines, it's just the tests on my machine that don't pick up the _filename - no, I spoke with my rear, it still fails there too, it's just that that test is skipped on GHA - still keeping it Draft since I needs to fix the tests

This is the test that fails at PyActiveStorage: https://github.com/NCAS-CMS/PyActiveStorage/actions/runs/22106044823/job/63888954418

I'll clean up and fix tests here, and add a test case, tomorrow 🍺

@codecov
Copy link

codecov bot commented Feb 18, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.62%. Comparing base (9f9e955) to head (b5d1d62).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
pyfive/h5d.py 75.00% 0 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (75.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   77.62%   77.62%   -0.01%     
==========================================
  Files          15       15              
  Lines        3124     3128       +4     
  Branches      497      499       +2     
==========================================
+ Hits         2425     2428       +3     
  Misses        573      573              
- Partials      126      127       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@valeriupredoi
Copy link
Collaborator Author

@bnlawrence @davidhassell it'd be good to have this in the sooner the better, one test fail in PyActiveStorage because of this (the test manipulating already-loaded datasets that need file names extracted from the Dataset's metadata)

Copy link
Collaborator

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

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

Hi V - looks good, with one minor change.
Cheers,
David

Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
@valeriupredoi
Copy link
Collaborator Author

Hi V - looks good, with one minor change. Cheers, David

mega, thanks very much @davidhassell - could you pls Approve? 🍻

@valeriupredoi valeriupredoi merged commit b8c2412 into main Feb 20, 2026
7 of 8 checks passed
@valeriupredoi valeriupredoi deleted the extract_filename_from_Dataset branch February 20, 2026 15:53
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.

Dataset loaded from S3 file returns _filename as "None" (str)

2 participants