oscal-cli icon indicating copy to clipboard operation
oscal-cli copied to clipboard

Enable command-line argument for pretty printing XML output (#268)

Open ermahesh opened this issue 6 months ago • 10 comments

Committer Notes

Fixes #268

This PR introduces a new command-line argument to enable pretty-printed XML output in the CLI tool.

Key Enhancements:

  • ✅ Added a --pretty-print CLI flag to toggle formatted XML output.
  • ✅ Integrated pretty-printing logic in the XML transformer flow.
  • ✅ Replaced FileOutputStream with Files.newOutputStream(...) for PMD compliance.
  • ✅ Added unit tests verifying formatted vs non-formatted output.
  • ✅ Verified behavior remains unchanged when the flag is not used.

Additional Notes:

  • This is my first contribution to this project. Kindly review and consider accepting this PR. I would be honored to be recognized as a contributor to OSCAL CLI.

Submission Checklist:

  • [x] Correct base branch selected.
  • [x] Maintainers allowed to edit the PR.
  • [x] Verified no conflicting open PRs.
  • [x] Squashed non-relevant commits.
  • [ ] All CI/CD checks passed.

Core Feature Checklist:

  • [x] Explained what the change does and why.
  • [x] Added relevant unit tests.
  • [x] Updated help text or usage documentation.

ermahesh avatar Jun 18 '25 03:06 ermahesh

@ermahesh - Thank you for your contributions. We will review the code and test the functionality.

iMichaela avatar Jun 23 '25 02:06 iMichaela

@selenaxiao-nist @iMichaela : I see the build fails for a different reason , Should we fixing that as part of this PR ?

ermahesh avatar Jun 25 '25 21:06 ermahesh

@ermahesh

  1. It appears that your pom.xml file is not the one we have in the develop branch, but rather the one from the main branch. XMLresolver version is defines in pom.xml
  2. Build and test actions will continue to fail due to Sonatype Maven changes. The team will test locally the proposed changes, and temporary remove all Maven pre-release tests.

iMichaela avatar Jun 25 '25 21:06 iMichaela

@selenaxiao-nist @iMichaela : I see the build fails for a different reason , Should we fixing that as part of this PR ?

Ideally - yes. Due to more recent policy on open-source, we might, for now, just temporarily remove the Maven release, and include guidance for users to build locally the liboscal-java and oscal-cli using the latest OSCAL release.

iMichaela avatar Jun 26 '25 03:06 iMichaela

@iMichaela What can I do to ensure this code is merged ?

ermahesh avatar Jun 30 '25 19:06 ermahesh

@iMichaela What can I do to ensure this code is merged ?

The PR cannot be merged as long as the tests performed by the CI/CD pipeline are not passing and the locally generated oscal-cli is tested. So far, the CI/CD pipeline has plenty of error which @selenaxiao-nist reproduced locally too when she tried to build the tool locally (without code release tests). She can provide more updates. You can work with her directly if you believe your code is not generating any errors when you build it locally. I am suggesting a v-meeting - please send an email to [email protected] to connect with our team.

iMichaela avatar Jun 30 '25 20:06 iMichaela

@ermahesh Please change PrettyPrinter class to use SaxonTransformerFactory. Refer to XMLOperations class I made this change

ermahesh avatar Jul 01 '25 18:07 ermahesh

@ermahesh Please change PrettyPrinter class to use SaxonTransformerFactory. Refer to XMLOperations class I made this change

Are you able to build this locally with no errors? Do your unit tests pass?

selenaxiao-nist avatar Jul 02 '25 14:07 selenaxiao-nist

It appears that pretty printing only works for profile resolution. Is there a reason that the feature was only implemented in that code path, or am I misreading the PR?

rsherwood-nist avatar Jul 04 '25 09:07 rsherwood-nist

It appears that pretty printing only works for profile resolution. Is there a reason that the feature was only implemented in that code path, or am I misreading the PR?

@rsherwood-nist This is my first contribution to the project — could you please help me understand if this fix needs to be applied in other parts of the codebase as well.

ermahesh avatar Jul 04 '25 22:07 ermahesh