-
Notifications
You must be signed in to change notification settings - Fork 160
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
Add String method for functions #1591
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides nitpicks, just one remark: It would be nice if a test was added for this (so that merging this does not decrease coverage).
lib/function.gi
Outdated
@@ -59,3 +59,16 @@ function(func) | |||
Append(result, " ) ... end"); | |||
return result; | |||
end); | |||
|
|||
|
|||
InstallMethod(String, "for a function, using string stream", [IsFunction] , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove space before comma?
lib/function.gi
Outdated
function(fun) | ||
local s, str; | ||
s := ""; | ||
str := OutputTextString(s,true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insert space after comma?
lib/function.gi
Outdated
|
||
|
||
InstallMethod(String, "for a function, using string stream", [IsFunction] , | ||
function(fun) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this odd indentation?
Codecov Report
@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
- Coverage 63.98% 63.97% -0.01%
==========================================
Files 995 995
Lines 325334 325344 +10
Branches 13032 13032
==========================================
- Hits 208155 208148 -7
- Misses 114381 114394 +13
- Partials 2798 2802 +4
|
69f3813
to
6886e13
Compare
@fingolfin I've addressed your comments. I've also made the original method a method for DisplayString. The String method now returns a version with normalised whitespace, which is |
lib/function.gi
Outdated
str := OutputTextString(s, true); | ||
PrintTo(str, fun); | ||
CloseStream(str); | ||
Add(s,'\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after comma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to me (though I have two super-nitpicks, but it's also OK to ignore them; I just won't be the one to hit merge while they are in, but steve or somebody else is most welcome to merge this)
lib/function.gi
Outdated
s := ""; | ||
str := OutputTextString(s, true); | ||
PrintTo(str, fun); | ||
CloseStream(str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super-nitpick, I am not expecting any action on it, but needed to get it off my chest: When I see "str" I think "string", but here it is "stream", while "s" is a string... This function is short, and so it's harmless, but I'd still rename at least one of them (probably str
to stream
, as I see strings more more often than streams, at least in GAP code)
A bit of a hack as it uses string streams rather than modifying the kernel Includes tests
6886e13
to
2ef363d
Compare
Fixed nitpicks. Just wait for the tests to make sure I didn't break something. |
A bit of a hack as it uses string streams rather than modifying the kernel