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 StSindarinContextInteractionModel>>#bindingOf: and #hasBindingOf: #72

Open
MarcusDenker opened this issue May 30, 2023 · 6 comments

Comments

@MarcusDenker
Copy link
Contributor

We should check bindingOf and hasBindingOf:

hasBindingOf: aString
	self bindings at: aString ifAbsent: [ 
		^ (context receiver class hasSlotNamed: aString) or: [ 
			  (context temporaryVariableNamed: aString) notNil ] ].
	^ true

bindingOf: aString
	^ self bindings at: aString ifAbsent: [ 
		  (context receiver class hasSlotNamed: aString)
			  ifTrue: [ context receiver class slotNamed: aString ]
			  ifFalse: [ context temporaryVariableNamed: aString ] ]
  • the order seems wrong. temps shadow inst vars, not the other way around
  • context understand #lookupVar: to do standard variable lookup, no need to test what kind to lookup first (if we follow the standard semantics
  • Do we really need to do variable lookup other than the local bindings at all here?
@adri09070
Copy link
Collaborator

From what I see, this class is in NewTools and is not used in the base Sindarin.

Im guess this is a part of an old debugger experiment

@MarcusDenker
Copy link
Contributor Author

ah, nice, I will remove it. yet another small thing simplified

@MarcusDenker
Copy link
Contributor Author

But it is used from StSindarinDebuggerScriptingPresenter>>#initializePresenters, which is NewTools-Sindarin-Tools

So does this mean the whole package can be removed?

@adri09070
Copy link
Collaborator

But this StSindarinDebuggerScriptingPresenter is not referenced anywhere in the system.

We should see with @StevenCostiou if we remove it as we may reuse it (or not) in the future

@MarcusDenker
Copy link
Contributor Author

Normally I would not care too much, but as we want to assess and then maybe simplify / improve the whole InteractionModel hierarchy and the #bindingOf: / lookupVar: APIs...

@adri09070
Copy link
Collaborator

adri09070 commented Jun 8, 2023

But this StSindarinDebuggerScriptingPresenter is not referenced anywhere in the system.

We should see with @StevenCostiou if we remove it as we may reuse it (or not) in the future

Actually, it is really used because StSindarinDebuggerScriptingPresenter is not referenced anywhere in the system BUT it is a debugger extension used by the debugger (via meta).

Anyway, I don't think something would go wrong if we simplify this method.
I don't know what you want to simplify exactly, but maybe things should be simplified there : https://github.com/pharo-spec/Chest/blob/master/src/Chest/ChestCodeScriptingInteractionModel.class.st

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

No branches or pull requests

2 participants