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

Remove unused templates in converter generators #77

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

galtm
Copy link
Collaborator

@galtm galtm commented Oct 20, 2023

Committer Notes

There doesn't seem to be any code anywhere in this repo or the original metaschema repo that applies templates with mode="make-step" or calls a template named "cast-prose-template". If these templates are holdovers from an earlier development phase, they can be deleted.

On the other hand, if they are part of a planned new development and will be in use eventually, then deletion is not the right action (in which case, close this PR without merging). @wendellpiez , do you know the status of these templates?

This PR is not urgent. I'm working on testing the converter generators, but I'll skip over these templates for now.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you squashed any non-relevant commits and commit messages? [instructions]
  • Do all automated CI/CD checks pass?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you included examples of how to use your new feature(s)?
  • Have you updated all website and readme documentation affected by the changes you made? Changes to the website can be made in the website/content directory of your branch.

@galtm galtm requested a review from a team as a code owner October 20, 2023 12:22
Copy link
Collaborator

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

I am guessing that 'make-step' is a relic of an earlier stage of development in which logic for the path-matching was not encapsulated as well in that wonderful function ... so 'make-step' did something similar to 'make-match` (only at the XPath step level not the match pattern level) and was overridden similarly at the level of the step, not of the match - only now not needed.

It's a theory!

In the src/testing folder there are XProcs for interactive testing of both converter pipelines and round-tripping those. Would those be a good thing to warm up, as an additional check against unintended regressions?

Also for consideration: maybe we should now be using xsl:mode declarations for all modes? (That was a new feature or didn't exist when the code was written.) This would help to document intent, as well as harden the runtime - for example if the mode were made to fail on no match.

Indeed replacing the moded template(s) with a mode declaration that produces a hard fail on-no-match could be another way to confirm the mode is never used by this or an importing/imported XSLT?

I'm less worried about removing an unused named template since that will produce the hard fail wanted if it's ever called, and it's even a no-op template.

@galtm
Copy link
Collaborator Author

galtm commented Oct 23, 2023

In the src/testing folder there are XProcs for interactive testing of both converter pipelines and round-tripping those. Would those be a good thing to warm up, as an additional check against unintended regressions?

Maybe. I don't think I need them just yet, so don't treat it as high priority on my account.

Also for consideration: maybe we should now be using xsl:mode declarations for all modes? (That was a new feature or didn't exist when the code was written.) This would help to document intent, as well as harden the runtime - for example if the mode were made to fail on no match.

Yes, that is a great idea for the reasons you mentioned.

Indeed replacing the moded template(s) with a mode declaration that produces a hard fail on-no-match could be another way to confirm the mode is never used by this or an importing/imported XSLT?

I can make that change locally and see if any failures come up as I work. Do you want me to push that change to this PR, too?

@wendellpiez
Copy link
Collaborator

Sure. sounds great, thanks!

@galtm
Copy link
Collaborator Author

galtm commented Oct 23, 2023

Added <xsl:mode> for the obsolete mode with this commit: f7ef8e8

(Adding <xsl:mode> for all the non-obsolete modes can be some other PR from whoever gets to it first.)

galtm added 2 commits March 8, 2024 17:25
There doesn't seem to be any code that applies templates with
mode="make-step" or calls a template named "cast-prose-template".
@galtm galtm force-pushed the remove-unused-code branch from f7ef8e8 to 36fb443 Compare March 8, 2024 22:26
Copy link

github-actions bot commented Mar 8, 2024

XSpec Test Results

  2 files  ±0  40 suites  ±0   0s ⏱️ ±0s
105 tests ±0  90 ✅ ±0  15 💤 ±0  0 ❌ ±0 
114 runs  ±0  99 ✅ ±0  15 💤 ±0  0 ❌ ±0 

Results for commit 36fb443. ± Comparison against base commit 0010340.

@galtm
Copy link
Collaborator Author

galtm commented Mar 8, 2024

I rebased my commit off the develop branch and force-pushed, to refresh this pull request. The checks ran and all passed.

@wendellpiez wendellpiez merged commit 8ada8a1 into usnistgov:develop Jun 4, 2024
3 checks passed
@galtm galtm deleted the remove-unused-code branch June 5, 2024 11:39
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.

3 participants