-
Notifications
You must be signed in to change notification settings - Fork 761
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
check for empty string in Object -writeDefFile #4953
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.
thanks! can you please add a test? we have assertException for checking throwing behavior
SCClassLibrary/Common/Core/Object.sc
Outdated
@@ -688,7 +688,7 @@ Object { | |||
StartUp.defer { // make sure the synth defs are written to the right path | |||
var file; | |||
dir = dir ? SynthDef.synthDefDir; | |||
if (name.isNil) { Error("missing SynthDef file name").throw } { | |||
if (name.isNil || name.asString.isEmpty) { Error("missing SynthDef file name").throw } { |
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.
it doesn't matter here but you should know currently it's slightly more performant to write || xyz
as or: { xyz }
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.
I didn't know that, thanks! Is this documented?
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.
i don't know, @jamshark70 is the one who always stresses this
be0e6e6
to
3f3be2a
Compare
thanks @brianlheim I added test, could you please take a look? I haven't seen examples of this check and wasn't sure if the message is good and whether |
3f3be2a
to
dcf0d0a
Compare
dcf0d0a
to
ab21a28
Compare
TestObject : UnitTest { | ||
|
||
test_writeDefFile_emptyName { | ||
this.assertException({ nil.writeDefFile("") }, Error, "nil.writeDefFile(\"\") should fail"); |
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 test should use an Object
, not nil
. if for some reason Nil.writeDefFile is implemented in the future this will be testing the wrong thing.
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.
I tried Object().writeDefFile("")
but that fails differently.
ERROR: Message 'writeDef' not understood.
RECEIVER:
Instance of Object { (0x1209cc898, gc=B8, fmt=00, flg=00, set=00)
instance variables [0]
}
ARGS:
Instance of File { (0x11ff23298, gc=B8, fmt=00, flg=00, set=02)
instance variables [1]
fileptr : nil
}
PROTECTED CALL STACK:
Meta_MethodError:new 0x11b0b7140
arg this = DoesNotUnderstandError
arg what = nil
arg receiver = an Object
Meta_DoesNotUnderstandError:new 0x11b0b9100
arg this = DoesNotUnderstandError
arg receiver = an Object
arg selector = writeDef
arg args = [ a File ]
Object:doesNotUnderstand 0x1168ad2c0
arg this = an Object
arg selector = writeDef
arg args = nil
ArrayedCollection:do 0x11e397340
arg this = [ an Object ]
arg function = a Function
var i = 0
Collection:writeDef 0x11e37cb00
arg this = [ an Object ]
arg file = a File
Function:prTry 0x11d25ff00
arg this = a Function
var result = nil
var thread = a Thread
var next = nil
var wasInProtectedFunc = false
CALL STACK:
DoesNotUnderstandError:reportError
arg this = <instance of DoesNotUnderstandError>
Nil:handleError
arg this = nil
arg error = <instance of DoesNotUnderstandError>
Thread:handleError
arg this = <instance of Thread>
arg error = <instance of DoesNotUnderstandError>
Object:throw
arg this = <instance of DoesNotUnderstandError>
Function:protect
arg this = <instance of Function>
arg handler = <instance of Function>
var result = <instance of DoesNotUnderstandError>
Function:doOnStartUp
arg this = <instance of Function>
Meta_StartUp:defer
arg this = <instance of Meta_StartUp>
arg object = <instance of Function>
Object:writeDefFile
arg this = <instance of Object>
arg name = "/Users/marcin/Library/Applic..."
arg dir = "/Users/marcin/Library/Applic..."
arg overwrite = true
Interpreter:interpretPrintCmdLine
arg this = <instance of Interpreter>
var res = nil
var func = <instance of Function>
var code = "Object().writeDefFile("")"
var doc = nil
var ideClass = <instance of Meta_ScIDE>
Process:interpretPrintCmdLine
arg this = <instance of Main>
^^ The preceding error dump is for ERROR: Message 'writeDef' not understood.
RECEIVER: an Object
and TestObject.run
(without the fix, to test the error) gives
-> TestObject
RUNNING UNIT TEST 'TestObject'
PASS: a TestObject: test_writeDefFile_emptyName - Object().writeDefFile("") should fail
Received exception of class 'DoesNotUnderstandError', with message: 'ERROR: Message 'writeDef' not understood.'
UNIT TESTS FOR 'TestObject' COMPLETED
There were no failures
I'd appreciate if you pointed me to the way to write this test. I'm just trying to get a minor fix in :)
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.
oh huh, that's a bit surprising, but i guess this is some sort of undocumented interface design. sorry, i didn't know. in that case, what you have is good, but could you leave a comment indicating that we don't test Object().writeDefFile directly because it's not fully implemented?
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.
Thanks! I added the comment, not sure if it should also have "FIXME" prefix or is it okay as it.
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.
thanks! this is good :)
Purpose and Motivation
Fixes #4873
Types of changes
To-do list