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

Replace IXML with SXML (for Cloud) #1188

Closed
wants to merge 4 commits into from
Closed

Conversation

sandraros
Copy link
Collaborator

@sandraros sandraros commented Feb 14, 2024

Big PR related to #632.

  • Classic abap2xlsx classes

    • Refer to ZCL/ZIF_EXCEL_XML... instead of CL/IF_IXML...
    • Use of data references with CREATE_OSTREAM_(X|C)STREAM methods
  • XML

    • ZCL/ZIF_EXCEL_XML(...) - Same (...) object type names as IXML classes and interface parts used by abap2xlsx (same method and parameter names, except it's a short list of what was used by abap2xlsx till now) - Parameter types refer to ZCL/ZIF_EXCEL_XML... instead of CL/IF_IXML... but run SXML. - implementations don't contain any reference to CL/IF_IXML...
      • contains many test classes to make sure that SXML and IXML results into identical XML
        • The tests run IXML only if ZCL_EXCEL_IXML exists (part of NOT_CLOUD package)
    • ZCL_EXCEL_XML_DOCUMENT
      • Same as CL_XML_DOCUMENT parts used by abap2xlsx
  • NOT_CLOUD

    • ZCL_EXCEL_IXML - contains references to CL/IF_IXML... (it's why it's in NOT_CLOUD)
      • used by ZCL_EXCEL_XML test classes to run abap2xlsx with IXML (if IXML is installed)

Potential issues:

  • Performance. Nothing optimized.
  • SXML is more strict than IXML and less permissive on positioning attributes, namespace declarations, etc.
  • OO design: Abuse of private instantiation/all friends.
  • Probably many I didn't see...

sandraros added 2 commits February 14, 2024 09:22
- Classic abap2xlsx classes
  - Refer to ZCL/ZIF_EXCEL_XML... instead of CL/IF_IXML...
  - Use of data references with CREATE_OSTREAM_(X|C)STREAM methods

- XML
  - ZCL/ZIF_EXCEL_XML(...)
    - Same (...) object type names as IXML classes and interface parts used by abap2xlsx (same method and parameter names, except it's a short list of what was used by abap2xlsx till now)
    - Parameter types refer to ZCL/ZIF_EXCEL_XML... instead of CL/IF_IXML... but run SXML.
    - implementations don't contain any reference to CL/IF_IXML...
	- contains many test classes to make sure that SXML and IXML results into identical XML
	  - The tests run IXML only if ZCL_EXCEL_IXML exists (part of NOT_CLOUD package)
  - ZCL_EXCEL_XML_DOCUMENT
    - Same as CL_XML_DOCUMENT parts used by abap2xlsx

- NOT_CLOUD
  - ZCL_EXCEL_IXML
    - contains references to CL/IF_IXML... (it's why it's in NOT_CLOUD)
	- used by ZCL_EXCEL_XML test classes to run abap2xlsx with IXML (if IXML is installed)

Potential issues:
- Performance. Nothing optimized.
- SXML is more strict than IXML and less permissive on positioning attributes, namespace declarations, etc.
- OO design: Abuse of private instantiation/all friends.
- Probably many I didn't see...
Copy link
Member

@AndreaBorgia-Abo AndreaBorgia-Abo left a comment

Choose a reason for hiding this comment

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

On 731:
grafik

Additional note: with the current "main" (both library and demos), checker reports some differences in 37, 38, and 15_01. The same checks on S4H (official SAP HANA appliance) bring up no findings. Disabling the strict comparison only works within the PR branch, so no chance to check on 731.

@sandraros
Copy link
Collaborator Author

Sorry, as Lars said, in the end IXML doesn't need to be replaced with SXML, it's only few classes CL_IXML to be replaced with CL_IXML_CORE, etc., all IF_IXML_* remain unchanged, so I remove my PR. It has to be recoded differently. Thanks for looking at it anyway! 👍

@sandraros sandraros closed this Feb 16, 2024
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.

None yet

3 participants