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

The 'pointfile' element is not part of the execution model XSD yet #622

Closed
hmeine opened this issue Jan 6, 2016 · 12 comments

Comments

@hmeine
Copy link
Contributor

commented Jan 6, 2016

I think @naucoin has introduced support for <pointfile> parameters, but Libs/CommandLineModules/Core/Resources/ctkCmdLineModule.xsd has not been updated yet.
As far as I know, that is the most official "spec" of CLIs' XML.

@naucoin

This comment has been minimized.

Copy link

commented Jan 6, 2016

Looking at the .xsd file, the pointfile needs to have a type that's a combo of fileType and pointType (for the coordinate system). I think I can make a new type that extends the fileType, calling it pointFileType.

naucoin pushed a commit to naucoin/CTK that referenced this issue Jan 6, 2016
Nicole Aucoin
BUG: add pointfile element
Add the pointfile element that has been added to the Slicer
executtion module[1] by extending the fileType. Also separated
out the coordinateSystem attribute so that it can be used in both the point
and pointFile types.

[1] Slicer/SlicerExecutionModel@608c7b0

Issue commontk#622
@naucoin

This comment has been minimized.

Copy link

commented Jan 6, 2016

@hmeine Take a look at this change and let me know if it works for you (I used an online validator). If so I can do a pull request:
naucoin@b8ee29f

Topic branch:
https://github.com/naucoin/CTK/tree/622-add-pointfile-element-to-execution-model-xsd

@hmeine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2016

To be honest, I have no good knowledge of XSD, but your addition looks totally sensible and is definitely useful as-is, if only for human understanding. :-)

@naucoin

This comment has been minimized.

Copy link

commented Jan 6, 2016

I was figuring it out as I went - I'll do the pull request to get some expert feedback!

@jcfr

This comment has been minimized.

Copy link
Member

commented Jan 6, 2016

After confirming that existing CTKCLI tests are passing and fixing typy in commit message. LGTM

Cc: @saschazelzer

naucoin pushed a commit to naucoin/CTK that referenced this issue Jan 6, 2016
Nicole Aucoin
BUG: Add pointfile element
Add the pointfile element that has been added to the Slicer
execution module[1] by extending the fileType. Also separated
out the coordinateSystem attribute so that it can be used in both the point
and pointFile types.

[1] Slicer/SlicerExecutionModel@608c7b0

Issue commontk#622
@naucoin

This comment has been minimized.

Copy link

commented Jan 6, 2016

@jcfr I ran 8 tests found via
ctest -R CmdLine
and all passed. Were those the tests that you meant?

@jcfr

This comment has been minimized.

Copy link
Member

commented Jan 6, 2016

@naucoin Indeed, these tests are expected to make use of the xsd schema.

@saschazelzer

This comment has been minimized.

Copy link
Member

commented Jan 7, 2016

The xsd changes look good to me. At least they do not invalidate current xml files. The tests will at most (I don't remember the details) verify that the xml files used within the tests still validate against the new schema.

If CTK itself is to support the new parameter type, it would need to be added to https://github.com/commontk/CTK/blob/master/Libs/CommandLineModules/Frontend/QtGui/Resources/ctkCmdLineModuleXmlToQtUi.xsl such that it is generated in the GUI. The change would be analogous to the file and geometry parameters.

@hmeine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2016

Oh, I just thought it was already supported in the CTK GUI generation. Maybe not, but that should be filed as a separate issue, I believe.

FWIW, it does LGTM, too.

@hmeine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2016

OK, who will merge this? @naucoin ? @jcfr ?

@jcfr

This comment has been minimized.

Copy link
Member

commented Jan 7, 2016

Integrated in 89639ec

@naucoin

This comment has been minimized.

Copy link

commented Jan 7, 2016

@hmeine New issue created: #626

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.