Skip to content

Document and Test PropertyTree Class #5921

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

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

bska
Copy link
Member

@bska bska commented Jan 28, 2025

In particular, add Doxygen-style documentation to the header file and add a simple unit test for the PropertyTree class interface.

While here, also add missing headers and remove those explicit template arguments that are not needed.

@bska
Copy link
Member Author

bska commented Jan 28, 2025

jenkins build this please

Copy link
Member

@akva2 akva2 left a comment

Choose a reason for hiding this comment

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

Two small comments.

#include <ostream>
#include <string>

#include <stddef.h>
Copy link
Member

Choose a reason for hiding this comment

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

we should probably change this to cstddef and instance for std::size_t. but okay to do later.

Copy link
Member Author

Choose a reason for hiding this comment

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

we should probably change this to cstddef and instance for std::size_t. but okay to do later.

I'll make a follow-up PR for that.

@@ -20,51 +20,136 @@
#ifndef OPM_PROPERTYTREE_HEADER_INCLUDED
#define OPM_PROPERTYTREE_HEADER_INCLUDED

#include <functional>
Copy link
Member

Choose a reason for hiding this comment

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

probably missing something but i don't see where this is required?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably missing something but i don't see where this is required?

We use std::less<> in the ptree definition.

@akva2
Copy link
Member

akva2 commented Jan 29, 2025

oh, and there are more TAD to be had (all of them can be deduced).

@bska
Copy link
Member Author

bska commented Jan 29, 2025

oh, and there are more TAD to be had (all of them can be deduced).

Right. I'll update the PR accordingly.

@bska bska force-pushed the document-and-test-property-tree branch from f73a027 to 4130127 Compare January 29, 2025 10:14
@bska
Copy link
Member Author

bska commented Jan 29, 2025

oh, and there are more TAD to be had (all of them can be deduced).

Right. I'll update the PR accordingly.

I've pushed an update to that effect now and I'll run a build test.

@bska
Copy link
Member Author

bska commented Jan 29, 2025

jenkins build this please

In particular, add Doxygen-style documentation to the header file
and add a simple unit test for the PropertyTree class interface.

While here, also add missing headers and prefer template argument
deduction over explicit template arguments.
@bska bska force-pushed the document-and-test-property-tree branch from 4130127 to efede0a Compare January 29, 2025 12:00
@bska
Copy link
Member Author

bska commented Jan 29, 2025

jenkins build this please

@bska
Copy link
Member Author

bska commented Jan 29, 2025

oh, and there are more TAD to be had (all of them can be deduced).

Right. I'll update the PR accordingly.

I've pushed an update to that effect now and I'll run a build test.

And latest update also fixes the "shadowing object" warning that the earlier version introduced... As the PR's approved and the build check is green I'll merge into master.

@bska bska merged commit b102ecd into OPM:master Jan 29, 2025
1 check passed
@bska bska deleted the document-and-test-property-tree branch January 29, 2025 12:38
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.

2 participants