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

Ensured that R script with unclosed brackets treated as incomplete R statement #7610

Merged
merged 1 commit into from
Jul 14, 2022
Merged

Ensured that R script with unclosed brackets treated as incomplete R statement #7610

merged 1 commit into from
Jul 14, 2022

Conversation

lloyddewit
Copy link
Contributor

@lloyddewit lloyddewit commented Jul 12, 2022

Fixes #7609

In the script window, R statements can be split across multiple lines for readability. When R-Instat parses the script, it parses line by line, and only executes the text when it judges that the R statement is complete.
It uses a set of heuristics to judge if the statement is complete (e.g. unclosed quotes, line ends in ',' etc.). This PR adds a check to ensure there that all open brackets have been closed. See issue #7609 for an example.

Note: The set of heuristics is quite simple but should cover most real-world examples. The RScript library parsing is much more sophisticated. However, it doesn't currently have a public function to check if the R script represents an incomplete R statement.
I suggest to stay with the simpler heuristics for now. If we find that we continue to expand the heuristics, then we should consider updating RScript and using this library instead (see issue #6744).

@lloyddewit lloyddewit added the bug label Jul 12, 2022
@lloyddewit
Copy link
Contributor Author

@rdstern Please could you test? thanks

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

Great. This now works perfectly with that example. I tried it both from the script window and also within the Edit > Script dialogue.
Two questions:
a) Does it also work with square brackets [ ] and also curly brackets { } which could also be used?
b) Now, if we open a bracket and then don't close it (or a quote) then I assume we can type anything and it will just continue "eating" the code, until I close. In interactive mode this could go on a long time. If I suspect that something needs closing is there anything I can type that will close a command - and give an error - but enable me interactively to not go on forever? I hope the question is clear?

@lloyddewit
Copy link
Contributor Author

lloyddewit commented Jul 13, 2022

@rdstern

Does it also work with square brackets [ ] and also curly brackets { } which could also be used?

It works with curly brackets but not square brackets. As I mentioned, currently there is a simple set of heuristics that aim to cover the most common cases. There are many aspects of R that is does not cover (e.g. single/double square brackets, single quotes, binary operators (apart from '+'), function declarations and other key words where the '{' is on the next line).

Now, if we open a bracket and then ...

It will continue adding the code to a buffer ('eating' the code) until the heuristics consider the statement closed. The user cannot see what's in the buffer, so there is a risk that they don't know how to close the statement. Currently, the only way to clear the buffer is to exit R-Instat.

Options:

  • accept the current situation
  • add a mechanism to allow the user to clear the buffer
  • add extra heuristics for the most common situations
  • implement a total solution.

I prefer the first or last option.

RStudio has a good approach. They have an editor and a console. The user can type commands directly into the console (or paste/ctrl-enter commands from the editor). The console executes the lines sequentially. If the statement is incomplete then it changes the console prompt from '>' to '+'. If the user presses 'escape', then it clears the buffer and resets the prompt to '>'.

image

We could consider replacing the script window with a console window, and then using RScript to cover the whole R syntax. R-Instat's console window should then be nearly as good as the one in RStudio (we may omit some of its advanced features). I estimate that this would take around 40 hours including the changes to the RScript library (this is a very rough initial estimate).

What's your view?
thanks

@rdstern
Copy link
Collaborator

rdstern commented Jul 14, 2022

@lloyddewit after discussion with @volloholic we are excited that you should go for your "total solution". It fits so well with having a script button on each dialogue, with our "halfway" dialogues and so on. Very exciting. And just 40 hours too - I mean that's less than 2 days!

That suggestion made my day yesterday, and the teaching in India, I was doing, has only confirmed that there will be powerful people who would welcome this facility. (The key person on that workshop, is a matlab user, who would quickly want to be using RStudio - i.e. commands, while having to start with R-Instat, because of its initial facilities in tidying and initial processing of the climatic data.)

This also fits my strategy, that we can never be the easiest R GUI, but we can be easy enough, and (by far) the most powerful! And letting people start using R so easily, and then including the set of packages so they can proceed very quickly, is attractive in itself.

@lloyddewit
Copy link
Contributor Author

@rdstern Thank you for your support and enthusiasm. We will go for it!
We can talk in person about the best approach and relative priorities.

@lloyddewit lloyddewit merged commit cc4eefb into IDEMSInternational:master Jul 14, 2022
@lloyddewit lloyddewit deleted the issue7609ScriptWinMultilineBug branch July 14, 2022 07:19
@lloyddewit
Copy link
Contributor Author

I merged this PR and will implement the R console against issue #6744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the running of code from the script window
2 participants