Fix #731, Improved error message when reading a data that is not defined in specification tree

[jmorice91]

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;
• in case of a change in pdi.h, this same change must be reflected in mock_pdi/pdi.h;
• in case of a new plugin, make sure the plugin issues the corresponding timer events;
• 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.

[Yushan-Wang]

Maybe just extend the error message:
e.g.

throw PDI::Type_error{"Can not read `{}' : buffer has unknown kind. Possible reason: buffer not defined in yaml (meta)data section", variable_name};

Or, as Iole suggested: use nulltype check before. c.f. scalar_datatype.cxx

[jbigot]

This is a fix for a regression that's impacting our users. We should get it out fast.

Please separate your review comments about problems that should be fixed before getting this in and improvements that can wait for a later release and should go in a dedicated issue.

[jbigot]

@jmorice91 : can you target a merge into v1.11 instead of main, please. You'll have to rebase --interactive to keep only the relevant commits. If it's not clear, let me know and I'll do it.

[Yushan-Wang]

This is a fix for a regression that's impacting our users. We should get it out fast.

Please separate your review comments about problems that should be fixed before getting this in and improvements that can wait for a later release and should go in a dedicated issue.

The default datatype is initialized to a scalar type. It's a "fault" on the user's part because they were using data that isn't defined in the yaml.

[jmorice91]

@jmorice91 : can you target a merge into v1.11 instead of main, please. You'll have to rebase --interactive to keep only the relevant commits. If it's not clear, let me know and I'll do it.

It is done. I have used "--onto" for my rebase.

[jmorice91]

To be sure, I have to merge in v1.12 or v1.11.

[jbigot]

This is a fix for a regression that's impacting our users. We should get it out fast.
Please separate your review comments about problems that should be fixed before getting this in and improvements that can wait for a later release and should go in a dedicated issue.

The default datatype is initialized to a scalar type. It's a "fault" on the user's part because they were using data that isn't defined in the yaml.

Is it? In #731 seems to be about array handling. In this PR, I see no test for arrays however. It would be good to add a test that matches the issue reported

[jbigot]

This is a fix for a regression that's impacting our users. We should get it out fast.
Please separate your review comments about problems that should be fixed before getting this in and improvements that can wait for a later release and should go in a dedicated issue.

The default datatype is initialized to a scalar type. It's a "fault" on the user's part because they were using data that isn't defined in the yaml.

Is it? In #731 seems to be about array handling. In this PR, I see no test for arrays however. It would be good to add a test that matches the issue reported

My bad, the issue seem to in fact be about a missing data: declaration. Everyone was telling me, but I am stubborn :D

[jbigot]

you could do a else if ( (*scalar_type ) == UNDEF_TYPE ) { ... } to handle this case as you were doing before and finish with a else { ... } where you throw an Impl_error for unimplemented scalar kinds

[jmorice91]

In this list of else, we are in a case of scalar_datatype. The last else correspond to the unknown case and we return a Type_error as the function nc_scalar_type in this file line 114.

[jbigot]

This is a fix for a regression that's impacting our users. We should get it out fast.

Please separate your review comments about problems that should be fixed before getting this in and improvements that can wait for a later release and should go in a dedicated issue.

So if I understand correctly, I was mistaken and this is not a bug fix, but an error message clarification. So I guess this doesn't have to go in a quick bugfix release and we can just merge it into main. My bad again

[Yushan-Wang]

This is a fix for a regression that's impacting our users. We should get it out fast.
Please separate your review comments about problems that should be fixed before getting this in and improvements that can wait for a later release and should go in a dedicated issue.

So if I understand correctly, I was mistaken and this is not a bug fix, but an error message clarification. So I guess this doesn't have to go in a quick bugfix release and we can just merge it into main. My bad again

No problem. We can then take some time to discuss suitable tests.

[jbigot]

Ready to merge from my point of view. Only a few small things to change:

• rebase on top of main
• adapt the changelog
• remove the inclusion of iostream unless really required

Add the UNDEF_TYPE/else cases if you feel this is a good thing to do