Skip to content
This repository was archived by the owner on Feb 26, 2025. It is now read-only.

Conversation

@alkino
Copy link
Member

@alkino alkino commented Feb 5, 2023

API can be discussed

@alkino alkino force-pushed the cornu/add_read_properties branch from 764fad5 to c599963 Compare February 5, 2023 10:25
@alkino
Copy link
Member Author

alkino commented Feb 5, 2023

I wonder if we should return tuple instead of having one getter for each value.

Should we do a static function instead?

static std::tuple<hsize_t, hsize_t> fromDataTransferProps(dxpl);

@alkino alkino force-pushed the cornu/add_read_properties branch from c599963 to eae2197 Compare February 5, 2023 10:28
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2023

Codecov Report

Merging #684 (52da3ee) into master (4e5e971) will increase coverage by 0.07%.
The diff coverage is 85.93%.

@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
+ Coverage   80.28%   80.35%   +0.07%     
==========================================
  Files          68       68              
  Lines        4113     4123      +10     
==========================================
+ Hits         3302     3313      +11     
+ Misses        811      810       -1     
Impacted Files Coverage Δ
include/highfive/H5PropertyList.hpp 93.33% <0.00%> (-4.17%) ⬇️
include/highfive/bits/H5PropertyList_misc.hpp 72.46% <85.45%> (+8.60%) ⬆️
include/highfive/H5Group.hpp 100.00% <100.00%> (+14.28%) ⬆️
include/highfive/bits/H5File_misc.hpp 95.23% <100.00%> (+7.89%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@alkino alkino force-pushed the cornu/add_read_properties branch from eae2197 to 76a812e Compare February 5, 2023 10:38
@alkino alkino force-pushed the cornu/add_read_properties branch from b59226d to c2b2583 Compare February 5, 2023 11:48
Copy link
Member

@matz-e matz-e left a comment

Choose a reason for hiding this comment

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

I think it looks pretty clean. The only place I'd return a pair is for the version bounds.

And we don't have too many other free functions, do we? So your current API seems fine to me.

@alkino alkino merged commit cd5cc8f into master Feb 10, 2023
@alkino alkino deleted the cornu/add_read_properties branch February 10, 2023 09:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants