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

About SpBrowserMethodVersionsCommand and SpCommand #1665

Open
Ducasse opened this issue Nov 23, 2024 · 4 comments
Open

About SpBrowserMethodVersionsCommand and SpCommand #1665

Ducasse opened this issue Nov 23, 2024 · 4 comments

Comments

@Ducasse
Copy link
Contributor

Ducasse commented Nov 23, 2024

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
Copy link
Contributor Author

Ducasse commented Nov 23, 2024

@estebanlm

@estebanlm
Copy link
Member

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

@Ducasse
Copy link
Contributor Author

Ducasse commented Nov 25, 2024

Ok I will do some PRs to improve the situation.

@Ducasse
Copy link
Contributor Author

Ducasse commented Nov 25, 2024

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.

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