Skip to content

592 improvement hdf5 dataset as regex #597

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

592 improvement hdf5 dataset as regex #597

wants to merge 15 commits into from

Conversation

@jmorice91
Copy link
Contributor

@jmorice91 jmorice91 commented Oct 1, 2025 •
edited
Loading

This PR correspond to fix the comment in the previous PR #583

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@jmorice91 jmorice91 self-assigned this Oct 2, 2025
Yushan-Wang
Yushan-Wang reviewed Oct 2, 2025
View reviewed changes
jbigot
jbigot reviewed Oct 28, 2025
View reviewed changes
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Nice improvement!

A few more comments. From my point of view, this PR should be high priority since it improves code already in main. Once merged, we can do a long awaited release.

benoitmartin88
benoitmartin88 requested changes Oct 28, 2025
View reviewed changes
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

Overall, this is great.
That being said, I think that a few little changes are needed.

jbigot
jbigot reviewed Oct 30, 2025
View reviewed changes
jbigot
jbigot reviewed Nov 3, 2025
View reviewed changes
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

oops, forgot to post thic comment apparently. Here it is

jbigot
jbigot reviewed Nov 3, 2025
View reviewed changes
Copy link
Member

@jbigot jbigot 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. Some remaining small remarks. I believe we're converging

jbigot
jbigot reviewed Nov 4, 2025
View reviewed changes
plugins/decl_hdf5/dataset_op.cxx
}
for (auto&& dsets_elem = dsets.begin(); dsets_elem != dsets.end(); ++dsets_elem) {
if (std::regex_match(dataset_name, dsets_elem->regex())) {
if (!dset_found.has_value()) {
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

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

you can also just do:

Suggested change
if (!dset_found.has_value()) {
if (!dset_found) {

but what you did is perfectly fine and maybe more explicit

jbigot
jbigot approved these changes Nov 4, 2025
View reviewed changes
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Great work!!!

This looks really good to me. This is really a great improvement, with a highly requested feature and great code quality. I'd say this is ready to ship

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbigot jbigot jbigot approved these changes

@JAuriac JAuriac Awaiting requested review from JAuriac

@Yushan-Wang Yushan-Wang Awaiting requested review from Yushan-Wang Yushan-Wang is a code owner

@benoitmartin88 benoitmartin88 Awaiting requested review from benoitmartin88

Requested changes must be addressed to merge this pull request.

Assignees

@jmorice91 jmorice91

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jmorice91 @jbigot @benoitmartin88 @Yushan-Wang