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

Add Group & Wrapper source XML line in errors #2404

Merged
merged 86 commits into from
Sep 5, 2023

Conversation

MelReyCG
Copy link
Contributor

@MelReyCG MelReyCG commented Apr 20, 2023

This PR aims to:

  • Add the ability to know the line of xml nodes & attributes.
  • Add metadata in Group & Wrapper in order to know from where they have been declared. When possible, this will allow to know in which source file and where exactly they have been defined (file line, offset), if not it will fallback to the Group hierarchy.
    This metadata will allow us to formulate error messages that can be clearer and faster to solve for users ( issue [EPIC] Improve feedback to user on XML errors #2320 ).
    This PR propose to put this metadata in a SourceContext abstract object, so if the Groups & Wrappers are loaded differently, we will be able to change the attached information.
  • Add a test to validate the Groups & Wrappers source file metadata, and also to verify if each read xml nodes and attributes correctly result in Group & Wrapper loading.

To impact the modifications on most GEOS files, 4 PR have been created to process multiple batches of files separately :

I planned to add another PR where I'll propose to properly wrap the following xmlWrapper types: xmlNode, xmlAttribute, xmlType

@MelReyCG MelReyCG self-assigned this Apr 20, 2023
@MelReyCG MelReyCG changed the title Add Group & Wrapper XML line in errors Add Group & Wrapper source XML line in errors Apr 20, 2023
…y/2320-xml-errors-feedback"

- using getSourceContext() instead of getPath()
…ack-3' into feature/rey/error-xml-line-2-errors-impacting
src/coreComponents/dataRepository/CMakeLists.txt Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/Group.cpp Outdated Show resolved Hide resolved
src/coreComponents/unitTests/xmlTests/testXMLFile.cpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/xmlWrapper.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/DataContext.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/DataContext.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/DataContext.cpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/DataContext.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/GroupContext.cpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/GroupContext.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/WrapperBase.cpp Outdated Show resolved Hide resolved
@TotoGaz
Copy link
Contributor

TotoGaz commented Aug 24, 2023

Do you think we've addressed all the comments and that this should be merged?

@MelReyCG
Copy link
Contributor Author

MelReyCG commented Aug 24, 2023

@TotoGaz Can you have a look on this one? #2404 (comment) (I can keep my initial proposal but I don't know if you are ok with it).
Apart from this one, and if the integratedTests run properly, yes I think this PR should be merged.

Copy link
Contributor

@TotoGaz TotoGaz 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 should be ready to get in.
@klevzoff if you want to check that the fixes you pointed are really fixed, you're welcome!

src/coreComponents/dataRepository/GroupContext.hpp Outdated Show resolved Hide resolved
src/coreComponents/dataRepository/GroupContext.cpp Outdated Show resolved Hide resolved
@TotoGaz
Copy link
Contributor

TotoGaz commented Sep 5, 2023

@MelReyCG I've put the PR in the merge queue.

@rrsettgast rrsettgast merged commit 74cee7e into develop Sep 5, 2023
@rrsettgast rrsettgast deleted the feature/rey/error-xml-line-2 branch September 5, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EPIC] Improve feedback to user on XML errors Error: SubRegion Doesn't have a child
7 participants