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

check for empty string in Object -writeDefFile #4953

Merged
merged 3 commits into from
May 24, 2020

Conversation

dyfer
Copy link
Member

@dyfer dyfer commented May 21, 2020

Purpose and Motivation

Fixes #4873

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • This PR is ready for review

@dyfer dyfer added the comp: class library SC class library label May 21, 2020
Copy link
Contributor

@mossheim mossheim left a 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

@@ -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 } {
Copy link
Contributor

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 }

Copy link
Member Author

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?

Copy link
Contributor

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

@dyfer dyfer force-pushed the topic/fix-writeDefFile branch from be0e6e6 to 3f3be2a Compare May 21, 2020 02:30
@dyfer
Copy link
Member Author

dyfer commented May 21, 2020

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 report arg should be true

@dyfer dyfer force-pushed the topic/fix-writeDefFile branch from 3f3be2a to dcf0d0a Compare May 21, 2020 02:33
@dyfer dyfer force-pushed the topic/fix-writeDefFile branch from dcf0d0a to ab21a28 Compare May 21, 2020 02:35
TestObject : UnitTest {

test_writeDefFile_emptyName {
this.assertException({ nil.writeDefFile("") }, Error, "nil.writeDefFile(\"\") should fail");
Copy link
Contributor

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.

Copy link
Member Author

@dyfer dyfer May 21, 2020

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 :)

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@mossheim mossheim left a 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 :)

@mossheim mossheim merged commit 9be3fe2 into supercollider:develop May 24, 2020
@patrickdupuis patrickdupuis mentioned this pull request Jul 6, 2020
4 tasks
mossheim added a commit that referenced this pull request Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Object:writeDefFile creates hidden files in the synthDefDir
2 participants