Check StSindarinContextInteractionModel>>#bindingOf: and #hasBindingOf:

pharo-spec / ScriptableDebugger

3 stars 12 forks source link

Check StSindarinContextInteractionModel>>#bindingOf: and #hasBindingOf: #72

Open MarcusDenker opened 1 year ago

MarcusDenker commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

MarcusDenker commented 1 year ago

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

So does this mean the whole package can be removed?

adri09070 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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