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

[c++] Refactor SOMAArray and SOMAGroup to inherit from SOMAObject #2124

Merged
merged 5 commits into from
Feb 12, 2024

Conversation

nguyenv
Copy link
Member

@nguyenv nguyenv commented Feb 12, 2024

Issue and/or context:

#2123

Changes:

  • DRY: refactor SOMAArray and SOMAGroup to inherit from SOMAObject
  • Set -mmacosx-version-min=11.0 to support usage of C++17 feature std::filesystem::path

These changes were picked out of #1817. I am opening smaller PRs to make it easier to review this time.

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Merging #2124 (cf0681d) into main (97625db) will increase coverage by 0.27%.
Report is 1 commits behind head on main.
The diff coverage is 59.18%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2124      +/-   ##
==========================================
+ Coverage   78.43%   78.70%   +0.27%     
==========================================
  Files         136      136              
  Lines       10759    10704      -55     
  Branches      209      211       +2     
==========================================
- Hits         8439     8425      -14     
+ Misses       2222     2177      -45     
- Partials       98      102       +4     
Flag Coverage Δ
libtiledbsoma 66.97% <59.18%> (-0.28%) ⬇️
python 91.38% <ø> (+0.64%) ⬆️
r 74.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 91.38% <ø> (+0.64%) ⬆️
libtiledbsoma 48.34% <59.18%> (-2.13%) ⬇️

@nguyenv nguyenv force-pushed the viviannguyen/refactor-inheritance branch from 6e37282 to 428fab3 Compare February 12, 2024 19:25
@nguyenv nguyenv requested a review from eddelbuettel February 12, 2024 19:35
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Looks good to me. One possible small enhancement but should work either way.

apis/r/configure Outdated Show resolved Hide resolved
Co-authored-by: Dirk Eddelbuettel <[email protected]>
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

🎉

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.

3 participants