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

UUID and message handling code copied from OSCAL repo #3

Merged
merged 3 commits into from
Mar 15, 2023
Merged

UUID and message handling code copied from OSCAL repo #3

merged 3 commits into from
Mar 15, 2023

Conversation

galtm
Copy link
Collaborator

@galtm galtm commented Oct 29, 2022

Copying random-util.xsl, uuid-method-choice.xsl, message-handler-xsl, and their XSpec test files from develop branch of usnistgov/OSCAL.

usnistgov/OSCAL#1158
usnistgov/OSCAL#1163
usnistgov/OSCAL#1175

The README.md files are new rather than copied. They are minimal for starters; I can make them more detailed if you want.

Copying random-util.xsl, uuid-method-choice.xsl, message-handler-xsl, and their XSpec test files
from develop branch of usnistgov/OSCAL.
usnistgov/OSCAL#1158
usnistgov/OSCAL#1163
usnistgov/OSCAL#1175
@aj-stein-nist
Copy link
Collaborator

@galtm I will review this soon but it has been made complicated that I can edit, but not pull down your branch because it is a branch of fork of a private repo (welcome to GitHub life that is not public/open by default). I have to download files one by one and recreate the PR, but expected feedback and a likely approval soon enough. Thanks for your patience!

@galtm
Copy link
Collaborator Author

galtm commented Feb 24, 2023

branch of fork of a private repo

@aj-stein-nist , I didn't realize there would be this complexity. If there's something I can do on my side to make this easier (in this PR or going forward), please let me know.

@aj-stein-nist
Copy link
Collaborator

branch of fork of a private repo

@aj-stein-nist , I didn't realize there would be this complexity. If there's something I can do on my side to make this easier (in this PR or going forward), please let me know.

You're all good! I understand these kinds of permission issues are not front of mind at all times. In the future, you can go to the following URL and invite aj-stein-nist and wendellpiez as collaborators.

https://github.com/galtm/xslt3-functions/settings/access

But for now, don't worry. I have ways of lazily working around it. I just want to test and understand your changes before just push-button approving. :-)

Copy link
Collaborator

@aj-stein-nist aj-stein-nist left a comment

Choose a reason for hiding this comment

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

So before final approval, we put the message handler and random UUID generator modules in separate namespaces. Does this presume for downstream consumer developers: you will import them separately, and these two functions and future additions of new functions like them later on will be namespaced separately? Or is the small number of functions make it unclear that this will long-term become like exslt with namespaced collections with functions organized around their common function/data type of focus?

Copy link
Collaborator

@aj-stein-nist aj-stein-nist left a comment

Choose a reason for hiding this comment

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

But also I want to thank you, @galtm, beyond my architectural approach questions, the code is very clean and well organized. I ran tests locally by recreating the new files in the same structure as the PR and testing them. It works quite well and explained the functions' functionality very well. Thanks so much!

image

@galtm
Copy link
Collaborator Author

galtm commented Feb 25, 2023

go to the following URL and invite aj-stein-nist and wendellpiez as collaborators.

Done!

@galtm
Copy link
Collaborator Author

galtm commented Feb 25, 2023

So before final approval, we put the message handler and random UUID generator modules in separate namespaces. Does this presume for downstream consumer developers: you will import them separately, and these two functions and future additions of new functions like them later on will be namespaced separately? Or is the small number of functions make it unclear that this will long-term become like exslt with namespaced collections with functions organized around their common function/data type of focus?

@aj-stein-nist , you're asking good questions. I put the message handler and random UUID generator modules in separate namespaces because the message handler is fairly generic. I thought an end user who's not interested in UUID generation might be interested in the message handler or in some future xslt3-function content that relies on the same message handler. Based on how you (and other interested parties on your team) see this repo evolving, you can tell me whether I should do something different. I don't mind revising the files to achieve a better long-term result. Thanks for your careful, hands-on review and comments!

@galtm
Copy link
Collaborator Author

galtm commented Feb 27, 2023

Outcome of discussion with @wendellpiez today: How about a namespace "http://csrc.nist.gov/ns/xslt3-functions" and prefix "nxf" across this whole repo? Other prefix options could be NXF, CSDF, X3F, or FX3. (Regardless of what we use here for a prefix, end users are free to pick their own prefix in their own code, as long as they map it to "http://csrc.nist.gov/ns/xslt3-functions" using their own namespace declaration.)

@aj-stein-nist
Copy link
Collaborator

Outcome of discussion with @wendellpiez today: How about a namespace "http://csrc.nist.gov/ns/xslt3-functions" and prefix "nxf" across this whole repo? Other prefix options could be NXF, CSDF, X3F, or FX3. (Regardless of what we use here for a prefix, end users are free to pick their own prefix in their own code, as long as they map it to "http://csrc.nist.gov/ns/xslt3-functions" using their own namespace declaration.)

Fair enough. I was asking because on the producer side, if I were a consumer, having each stylesheet of functions in a different namespace as it seems now implies they are separate from one another and not a unified library. As maybe the first consumer, I wanted to better understand that choice.

@aj-stein-nist
Copy link
Collaborator

Outcome of discussion with @wendellpiez today: How about a namespace "http://csrc.nist.gov/ns/xslt3-functions" and prefix "nxf" across this whole repo? Other prefix options could be NXF, CSDF, X3F, or FX3. (Regardless of what we use here for a prefix, end users are free to pick their own prefix in their own code, as long as they map it to "http://csrc.nist.gov/ns/xslt3-functions" using their own namespace declaration.)

@galtm and @wendellpiez, is this a change you can make in the next week or you think it will require more time? No pressure, just let me know, thanks!

@galtm
Copy link
Collaborator Author

galtm commented Feb 27, 2023

It is easy and I can do it in the next week, once there is a consensus about the namespace and prefix.

@wendellpiez
Copy link
Collaborator

x3f might be my favorite so far. While I suggested the all-upper case convention, I won't nominate it (this time).

@aj-stein-nist
Copy link
Collaborator

x3f might be my favorite so far. While I suggested the all-upper case convention, I won't nominate it (this time).

I am happy we move along with a uniform namespace and the prefix I will be less picky about, except for keeping it lower case. I feel that is consistent enough as of now.

On a related topic, @wendellpiez, part of the AC for this issues in sprint related to this work mention the evaluation of using these functions with xsl:package directives. Is the path forward, given the current code and namespace updates, clear on that front?

@wendellpiez
Copy link
Collaborator

This could be an evaluation spike but no one has yet presented the use case or requirement for using xsl:package. (We are simply aware of it as a feature designed for supporting libraries.) Unless we plan to compile and deliver package builds it is not clear what the utility is, for us. Free tools (tmk) do not currently take any advantage of xsl:package over xsl:stylesheet.

In short, this is something we could do but it is unclear what the benefit would be (at present). Happy to contemplate further if anyone differs.

@aj-stein-nist
Copy link
Collaborator

This could be an evaluation spike but no one has yet presented the use case or requirement for using xsl:package. (We are simply aware of it as a feature designed for supporting libraries.) Unless we plan to compile and deliver package builds it is not clear what the utility is, for us. Free tools (tmk) do not currently take any advantage of xsl:package over xsl:stylesheet.

In short, this is something we could do but it is unclear what the benefit would be (at present). Happy to contemplate further if anyone differs.

OK, then the answer is no. Once this is merged, will it be simple for another member of the team to swap out the inline version in OSCAL and replace it with this version through an xsl:import statement or other means with 1) clear acceptance criteria and 2) your help? I would like others to understand this work to the best extent possible, so I want us to plan for it. I hope that helps explain the questions. Thanks!

@wendellpiez
Copy link
Collaborator

Indeed, it should be possible to 'just swap it' subject to changes in both import path and namespaces.

A separate question is whether imports can/should reach across the Internet since won't that confound CORS policies? Won't we need to put subordinate code into submodules to avoid this (and in any case for dependency management)?

@galtm
Copy link
Collaborator Author

galtm commented Mar 1, 2023

x3f might be my favorite so far. While I suggested the all-upper case convention, I won't nominate it (this time).

I am happy we move along with a uniform namespace and the prefix I will be less picky about, except for keeping it lower case. I feel that is consistent enough as of now.

I'll proceed to use "http://csrc.nist.gov/ns/xslt3-functions" and prefix x3f across this whole repo.

Reaching across the Internet for an imported library seems strange. With the submodule approach, a user who clones the OSCAL repo locally can easily get a local copy of this code, too.

@galtm
Copy link
Collaborator Author

galtm commented Mar 2, 2023

@aj-stein-nist and @wendellpiez : New commit ff9c513 changes the namespace in the UUID and message handling functions. I ran all the XSpec tests in Oxygen 25, including the long-running tests in random-util.xspec that are marked pending="To save time, run only when needed".

Next steps...
I was planning to push an additional commit to use this new namespace in the semantic version function, too. However, it occurred to me that the full name x3f:compare might be misleading because it's a generic name with non-generic functionality (i.e., semantic version numbers are not generic). A more descriptive name like x3f:compare-versions could clarify the meaning of the function. Disadvantages are that we lose the parallel with the XPath compare function for strings, and we have to use a little more care when updating OSCAL code to use this library. In my opinion, the benefits in clarity outweigh these disadvantages.

  1. What are your thoughts on the name of the semantic version comparison function?
  2. Should I use a separate pull request to change the namespace (and maybe local name) of that function, to avoid cluttering this pull request?

Thanks!

@wendellpiez
Copy link
Collaborator

I agree on the clearer name. It could even be semver-compare() if you think the neologism is okay.

Will defer to @aj-stein-nist on the PR question.

Thanks @galtm !

@galtm
Copy link
Collaborator Author

galtm commented Mar 6, 2023

Let's use semver-compare. I'll wait to hear about the PR question when you get a chance, @aj-stein-nist .

@aj-stein-nist
Copy link
Collaborator

aj-stein-nist commented Mar 15, 2023

Let's use semver-compare. I'll wait to hear about the PR question when you get a chance, @aj-stein-nist .

So are we going to merge this and handle the semver work later? I am fine with this as-is. I would prefer we move the semver work be done separately if possible (per the acceptance criteria of this issue).

@wendellpiez
Copy link
Collaborator

@galtm asks above whether to do the name change in a new PR, and that seems reasonable. I made #4 for that.

Assuming that looks good, we can merge this, I think.

@aj-stein-nist
Copy link
Collaborator

@galtm asks above whether to do the name change in a new PR, and that seems reasonable. I made #4 for that.

Assuming that looks good, we can merge this, I think.

I saw your update on 4, thanks for the quick turnaround. We can merge this then.

@aj-stein-nist aj-stein-nist merged commit 58ee15a into usnistgov:main Mar 15, 2023
@galtm galtm deleted the uuid-util branch November 20, 2023 14:33
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