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

use [[(]]) instead of <<(>>) for analysis plots to correct invalid xml syntax #362

Merged
merged 7 commits into from
Jan 12, 2023

Conversation

lobis
Copy link
Member

@lobis lobis commented Jan 12, 2023

lobis Ok: 75

As the title says we cannot use << or >> in the xml (or rml) files if we want it to be valid xml, even though some parsers can tolerate it.

I replaced them by square brackets but any suggestion is welcome.

This issue was discovered when running a tool to check for valid xml files.

@jgalan
Copy link
Member

jgalan commented Jan 12, 2023

There are other places where we use &gt; would it make sense to search for a different nomenclature?

    <addTask command="restplot-&gt;PlotCombinedCanvas()" value="ON"/>

We might use: restplot.PlotCombinedCanvas().

@lobis
Copy link
Member Author

lobis commented Jan 12, 2023

There are other places where we use &gt; would it make sense to search for a different nomenclature?

    <addTask command="restplot-&gt;PlotCombinedCanvas()" value="ON"/>

We might use: restplot.PlotCombinedCanvas().

I am not familiar at all with TRestAnalysisPlots can you explain further please? I don't understand the issue.

@lobis
Copy link
Member Author

lobis commented Jan 12, 2023

I think I fixed all the problems, I don't think I broke anything with this change but I am not 100% sure as I am not familiar with TRestAnalysisPlots.

@lobis lobis requested a review from jgalan January 12, 2023 11:55
@lobis lobis added the invalid This doesn't seem right label Jan 12, 2023
@lobis lobis marked this pull request as ready for review January 12, 2023 11:59
@nkx111
Copy link
Member

nkx111 commented Jan 12, 2023

[[xxx]] is a better replacing mark for me

@lobis lobis requested a review from nkx111 January 12, 2023 13:22
@nkx111
Copy link
Member

nkx111 commented Jan 12, 2023

There are other places where we use &gt; would it make sense to search for a different nomenclature?

    <addTask command="restplot-&gt;PlotCombinedCanvas()" value="ON"/>

We might use: restplot.PlotCombinedCanvas().

We can update TRestTask.cxx to support also restplot.PlotCombinedCanvas().

@jgalan
Copy link
Member

jgalan commented Jan 12, 2023

There are other places where &gt; is also used, for example inside the globalCut definition, inside TRestAnalysisPlot, the condition value can be something like >10.

When we really want to say greater than, how we could replace this symbol?

Inside TRestDataSet I added 3-new optional fields, such as greaterThan, lowerThan, equals, so that we don't need to introduce the > character.

Copy link
Member

@jgalan jgalan left a comment

Choose a reason for hiding this comment

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

Looks fine to me

source/framework/core/src/TRestAnalysisPlot.cxx Outdated Show resolved Hide resolved
source/framework/core/src/TRestAnalysisTree.cxx Outdated Show resolved Hide resolved
source/framework/core/src/TRestAnalysisTree.cxx Outdated Show resolved Hide resolved
source/framework/core/src/TRestAnalysisTree.cxx Outdated Show resolved Hide resolved
source/framework/core/src/TRestAnalysisTree.cxx Outdated Show resolved Hide resolved
@lobis lobis merged commit 3680798 into master Jan 12, 2023
@lobis lobis deleted the lobis-xml branch January 12, 2023 14:27
@jgalan
Copy link
Member

jgalan commented Feb 9, 2023

Hi @lobis and @juanangp merging this PR made the rawlib validation pipeline fail.

rest-for-physics/rawlib#95

It is possible to trigger the pipelines of few key libraries at the PR in the framework?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants