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

Draft: Input Parsing Refactor / Generic Input Extension Mechanisms #3180

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

wrtobin
Copy link
Collaborator

@wrtobin wrtobin commented Jun 20, 2024

No description provided.

@wrtobin wrtobin added type: feature New feature or request type: cleanup / refactor Non-functional change (NFC) changes XML input labels Jun 20, 2024
@wrtobin wrtobin self-assigned this Jun 20, 2024
@untereiner
Copy link
Contributor

untereiner commented Jun 21, 2024

Hello @wrtobin ,
I was looking at you draft and I don't understand the objective of the modifications for xml inclusions.
Why are the included xml files more inlaid in the Group concept ?
What is the concept of Generic Input Extension

I think this work is also related to this discussion #3073

I would be in favor of keeping things separated:

  • on one side xml files (even splitted files), the entire xml tree can be reconstructed and validated
  • on the other side the elements of the tree that can be instantiated as objects by geos.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jun 27, 2024

Part of this work is attempting to separate input file format dependence / parsing from internal group/attribute instantiation and definition.

There is still work to do (clearly) but this includes abstracting inclusion to the point that files of arbitrary (but supported) hierarchical input formats can include other files of arbitrary (but supported) hierarchical input formats.

Whether we want to fully allow that is a different concern from designing a system that is componentized in such a way to facilitate trivially supporting that.

To accomplish that inclusion needs to be independent of document type, where it sits right now ( the current implementation ) is mostly a result of iteratively removing it from the xmlwrapper, rather than a hard decision of how we should actually accomplish this.

@untereiner
Copy link
Contributor

The separation of file format / groups is a very good idea
But, file inclusion is a file thing and is more or less supported depending on the chosen format. It should then be handled at the file level IMHO. The input parser should take as input a complete document to parse.

If follow you remark I'd say it should be possible to include yaml files in json files and theses files in xml files. But that's a bit weird.

And yes the work sits in the xmlwrapper but I am concerned about the handling of these includes.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jun 28, 2024

The inclusion modification is currently the part of this work I like the least, yeah.

While I don't necessarily want to support inclusion of files of different formats, my intent is to implement things in a generic enough way that such a thing is possible (though not supported), since really we're just working with a bunch of abstract trees in all cases.

I'm amenable to reworking it and seeing if putting inclusion processing into document abstractions makes things cleaner.

Copy link
Contributor

@MelReyCG MelReyCG left a comment

Choose a reason for hiding this comment

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

A few comments

auto & knownChildNames = childNamesMap[ dataParent.getName() ];
document_node_pos docNodePos = m_document.getNodePosition( docNode );
GEOS_ERROR_IF( std::find( knownChildNames.begin(), knownChildNames.end(), dataNodeName ) != knownChildNames.end(),
GEOS_FMT( "Error: An Input block cannot contain children with duplicated names.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

An input node ?

Comment on lines +400 to +405
using PreprocessOnly = InputProcessor< xmlWrapper::xmlDocument,
Group,
TerseSyntax< xmlWrapper::xmlDocument, Group >,
Declaration< xmlWrapper::xmlDocument, Group >,
TerseSyntax< xmlWrapper::xmlDocument, Group > >;

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the fact that we could be agnostic about the type & process of the input file in Group

Copy link
Contributor

Choose a reason for hiding this comment

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

if the xml would be versioned, we could add a LegacyCompatibility phase that would transform the data model to the last version.

inputExtension::InputExtender< document_node > m_inputExtender;
};

template < typename DocumentType, typename DataNode, typename... Phases >
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
template < typename DocumentType, typename DataNode, typename... Phases >
template < typename DocumentType, typename DataNode, typename... InputPhases >


void xmlDocument::addIncludedXML( xmlNode & targetNode, int const level )
Copy link
Contributor

Choose a reason for hiding this comment

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

As you guys already said about the moving / deletion of xmlDocument::addIncludedXML(), I also think it is up to the file format to define a way to include files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes XML input type: cleanup / refactor Non-functional change (NFC) type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants