aiida-core icon indicating copy to clipboard operation
aiida-core copied to clipboard

Add `is_file` property to `RemoteData`

Open GeigerJ2 opened this issue 9 months ago • 3 comments

GeigerJ2 avatar Mar 25 '25 14:03 GeigerJ2

Codecov Report

:x: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review. :white_check_mark: Project coverage is 78.31%. Comparing base (660fec7) to head (7b80993). :warning: Report is 124 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/orm/nodes/data/remote/base.py 87.50% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6807      +/-   ##
==========================================
+ Coverage   78.31%   78.31%   +0.01%     
==========================================
  Files         566      566              
  Lines       42762    42770       +8     
==========================================
+ Hits        33484    33491       +7     
- Misses       9278     9279       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Mar 25 '25 15:03 codecov[bot]

Hi @agoscinski, thanks for the input! Agree with all points, this was a very low-effort PR :D Though, the more I think about this, the more I think we should solve this issue properly by introducing a RemoteSinglefileData class... If RemoteData is assumed to point to a directory throughout the whole code base, it might otherwise give us lots of headache. In that case, rather than adding the is_file check to RemoteData like I did in this PR, along with introducing RemoteSinglefileData, I'd disallow even constructing a RemoteData with a path to a file in the first place. What do you think?

GeigerJ2 avatar Mar 27 '25 08:03 GeigerJ2

I agree. Probably we should create a base class for RemoteData and RemotesinglefileData that contains the get_authinfo and get_size_on_disk methods.

Something I have not thought through because I am not sure how the migration of the database works: Maybe we can create a new class RemoteFolderData that replaces RemoteData so we can use RemoteData as base class. It is not a breaking change if a database migration fixes it. In this case we just need change the class by reconstruction the objects in the migration process.

agoscinski avatar Apr 07 '25 10:04 agoscinski

Closing this, as we better implement a proper structure, as outlined by @agoscinski in his last comment.

GeigerJ2 avatar Nov 07 '25 12:11 GeigerJ2