Make GAP.EvalString more convenient #163
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
Rename the current GAP.EvalString to GAP.EvalStringEx (for "Ex"tended), added a new GAP.EvalString which only returns the value of the last command, and which also automatically appends a semicolon to the input string.
Codecov Report
@@ Coverage Diff @@
## master #163 +/- ##
==========================================
+ Coverage 54.39% 54.45% +0.05%
==========================================
Files 37 37
Lines 1682 1684 +2
==========================================
+ Hits 915 917 +2
Misses 767 767
|
sebasguts
reviewed
Nov 22, 2018
| res = ccall( :GAP_EvalString, MPtr, | ||
| (Ptr{UInt8},), | ||
| cmd ); | ||
| return res | ||
| end | ||
|
|
||
| function EvalString( cmd :: String ) | ||
| res = EvalStringEx(cmd * ";") |
There was a problem hiding this comment.
Are you sure this is an sensible convention?
I am not sure it makes sense here, but in GAP you can end statements with ; or ;;. If you now put ;; at the end of a statement, this command does not return a value. Is that intentional?
There was a problem hiding this comment.
Yes, quite intentional:
- if you want to shoot yourself in the foot, you can always do that, and e.g. end the string with a command that returns nothing; or add
;;;;;to the end (which amounts more or less the same); or do something else that's crazy. You can do so with the existingEvalStringcommand do - I though about doing something like "check if there is a trailing semicolon and only add one if there is not" -- but then you can continue this game, and ask "what if the user put three semicolons at the end, should I remove some" etc.
- Adding a semicolon is also what GAP's own
EvalStringdoes, so now the two match in their behavior.
There was a problem hiding this comment.
Yeah, I agree that more or less all solutions have pitfalls. If GAP's EvalString method behaves like this, it is okay. Stupid question, why don't we make EvalString just
EvalString( x ) = GAP.Globals.EvalString( julia_to_gap( x ) )
This way we can have the exact same behaviour?
sebasguts
approved these changes
Nov 23, 2018
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.
Rename the current GAP.EvalString to GAP.EvalStringEx (for "Ex"tended), added
a new GAP.EvalString which only returns the value of the last command, and
which also automatically appends a semicolon to the input string.