Spec icon indicating copy to clipboard operation
Spec copied to clipboard

About SpBrowserMethodVersionsCommand and SpCommand

Open Ducasse opened this issue 1 year ago • 4 comments

Hi esteban

I have several questions:

  • why this class is not packaged in NewTools?
  • Should not be named StBrowser
  • What about SpToolCommand?
  • then

Can we define application as

application

     ^ self context application 

as in IceTipCommand, StCommand, StFileBrowserCommand, .....

  • should not all the tool commands inherit from the same superclass?

And then we could turn

execute
	| target |

	target := self target.
	Smalltalk tools versionBrowser
		browseVersionsForClass: target methodClass
		selector: target selector

into

execute
	| target |

	target := self target.
	self application tools versionBrowser
		browseVersionsForClass: target methodClass
		selector: target selector

Ducasse avatar Nov 23 '24 15:11 Ducasse

@estebanlm

Ducasse avatar Nov 23 '24 15:11 Ducasse

Hi esteban

I have several questions:

* why this class is not packaged in NewTools?

I think this is because of historical reasons. I do agree it belongs more to newtools than here.

* Should not be named StBrowser

if moved, yes.. StBrowserEtc.

* What about SpToolCommand?

Same, it should probably need to be elsewhere, but we have the SpCodePresenterthere annoying :) IMO, SpCodePresenter is complex enough to have its own repository, also because it is in middle way between spec and newtools (as it is a presenter that is specifically made for doing pharo tools).

* then

Can we define application as

application

     ^ self context application 

Indeed. in fact I think there are already some commands define it, I would think adding it to SpCommand would not be wrong.

as in IceTipCommand, StCommand, StFileBrowserCommand, .....

* should not all the tool commands inherit from the same superclass?

... maybe ? :P

And then we could turn

execute
	| target |

	target := self target.
	Smalltalk tools versionBrowser
		browseVersionsForClass: target methodClass
		selector: target selector

into

execute
	| target |

	target := self target.
	self application tools versionBrowser
		browseVersionsForClass: target methodClass
		selector: target selector

estebanlm avatar Nov 25 '24 11:11 estebanlm

Ok I will do some PRs to improve the situation.

Ducasse avatar Nov 25 '24 11:11 Ducasse

I would not create another repo for SpCodePresenter we can move it to new tools. I will introduce a new subclass of SpToolCommand (because find and replace can stay an SpCommand). I will rename all the commands into St Once this is done I will see how to move this to NewTools.

Ducasse avatar Nov 25 '24 20:11 Ducasse