R Syntax: split analysis and analysisForm #5764
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JorisGoosen
reviewed
Dec 19, 2024
| return false; | ||
| jsonResult["options"] = jsonOptions; | ||
| jsonResult["error"] = fq(getError()); | ||
| return tq(jsonResult.toStyledString()); |
There was a problem hiding this comment.
Why not return the Json value then?
There was a problem hiding this comment.
In the jaspQMLR example, the QML form is just a QuickItem object, so I call the parseOptions with the QMetaObject::invokeMethod method:
https://github.com/jasp-stats/jaspQmlR/blob/a7a93a2f6fa103d47560c1ed541e9c252eed3671/src/qmlR.cpp#L208
In this way I don't need to load the AnalysisFormBase header in the jaspQMLR package.
I don't think I can pass a Json::Value argument in the InvokeMethod, so I thought just send the string, and reset it as Json object afterwards.
Move the initColumnWithStrings from DataSetPackage to DataSet. Add parseOptions in AnalysisForm Update generateWrapper so that it can be called from the
boutinb
force-pushed
the
prepareRSyntax
branch
from
cea99e9 to
6083ea2
Compare
JorisGoosen
reviewed
Dec 20, 2024
QMLComponents/analysisform.cpp
Outdated
| Json::Value jsonOptions; | ||
| Json::Value jsonResult(Json::objectValue); | ||
|
|
||
| QJsonDocument doc = QJsonDocument::fromJson(options.toUtf8()); |
There was a problem hiding this comment.
Why is this step here?
It seems that the whole QJsonDocument
JorisGoosen
reviewed
Dec 20, 2024
JorisGoosen
reviewed
Dec 20, 2024
JorisGoosen
reviewed
Dec 20, 2024
JorisGoosen
reviewed
Dec 20, 2024
JorisGoosen
reviewed
Dec 20, 2024
JorisGoosen
approved these changes
Dec 20, 2024
JorisGoosen
pushed a commit
that referenced
this pull request
Jan 7, 2025
* Prepare R Syntac Move the initColumnWithStrings from DataSetPackage to DataSet. Add parseOptions in AnalysisForm Update generateWrapper so that it can be called from the
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As described in the JASP Architecture for R Syntax document, we need to split the analysis and analysisForm objects. For this the AnalysisFormBase is added (AnalysisBase existed already), the Base objects contain all calls from Analysis to AnalysisForm and vice-versa.
The initColumnWithString is function is moved to the DataSet object so that the RDataFrame importer can use it.
The parseOptions function in the AnalysisForm is already added.