The Inbox: Tools-ct.1054.mcz

Am 07.05.2021 07:35:27 schrieb Tom Beckmann <[hidden email]>:

Hi Chris,

speaking from experience with an extension like this: I started out with a script that reformatted all methods in my package (it was a good idea) and moved on to using something like this proposed extension.

For further context, I have gotten used to and comfortable with the idea that formatting is just busy work in 95% of cases that I'd like to spend on something productive rather than moving whitespace.

Undoubtedly, using a pretty printer on most trunk code is infeasible, as each method/class/package currently follows different intricacies of secondary notation. Additionally, there are of course some "special" (from the POV of our pretty printer) formatting choices that authors deliberately chose to make a point about the code. This type of secondary notation, where it's actually valuable, is I think quite common in trunk code, but exceedingly uncommon in code I produce in the daily business.

I don't think anyone currently even considers applying a pretty printer against all trunk code, for various reasons.

Since it's a preference I would give the proposed change a +1. It supports a valuable workflow that I believe is slowly becoming feasible in Squeak. The Ctrl+Shift+S shortcut could even be inverted when the preference is active so that you can keep formatting idiosyncrasies where it's appropriate.

It may be important to note that we are working on having a pretty printer understand common Smalltalk idioms and format those accordingly. We are also planning to try and maintain deliberate choices, such as empty lines, strides in array formatting, or comment positions.

If you've never tried programming in an ecosystem where there's a well-accepted standard for code style that can be automatically applied, I'd recommend you give it a shot. At least for me, it allowed performing changes more directly (no tedious cleanup each time I want to look at an intermediate or final state of a change) and saved a good chunk of brain power that I could invest elsewhere :) 

Best,

Tom

On Fri, May 7, 2021, 02:25 Chris Muller <[hidden email]> wrote:

-1.  The IDE should not break the boundaries of roles between the
human and IDE.  IOW, it should maintain explicit gesture separation
between what the human crafted, and what is stored in the system.  If
this was really a good idea, why not write a script to simply format
all methods in the whole system? (answer: because I'm sure you agree
that's a bad idea).  Or why not just use #browseWithPrettyPrint?

There is already a hot-key for pretty-print (Shift+Cmd+S), so you can
obtain the same effect with virtually no extra effort if you want to.




On Thu, May 6, 2021 at 4:40 PM <[hidden email]> wrote:
>
> A new version of Tools was added to project The Inbox:
> http://source.squeak.org/inbox/Tools-ct.1054.mcz
>
> ==================== Summary ====================
>
> Name: Tools-ct.1054
> Author: ct
> Time: 6 May 2021, 11:40:27.54561 pm
> UUID: edc189dc-7bb9-974a-9aa3-4760e7e67239
> Ancestors: Tools-mt.1053
>
> Proposal: Adds a new preference #acceptWithPrettyPrint that, if enabled, automatically pretty-prints every message before accepting it in a code holder. When used together with the preferences #browseWithPrettyPrint (and maybe also #diffsWithPrettyPrint), given a good pretty-printer such as PoppyPrint, this has the potential to make your journey through Squeak even prettier. :-)
>
> =============== Diff against Tools-mt.1053 ===============
>
> Item was changed:
>   ----- Method: Browser>>defineMessageFrom:notifying: (in category 'message functions') -----
>   defineMessageFrom: aString notifying: aController
>         "Compile the expressions in aString. Notify aController if a syntax error occurs. Install the compiled method in the selected class classified under  the currently selected message category name. Answer the selector obtained if compilation succeeds, nil otherwise."
>         | selectedMessageName selector category oldMessageList selectedClassOrMetaClass |
>         selectedMessageName := self selectedMessageName.
>         oldMessageList := self messageList.
>         selectedClassOrMetaClass := self selectedClassOrMetaClass.
>         contents := nil.
>         selector := (selectedClassOrMetaClass newParser parseSelector: aString).
>         (self metaClassIndicated
>                 and: [(selectedClassOrMetaClass includesSelector: selector) not
>                 and: [Metaclass isScarySelector: selector]])
>                 ifTrue: ["A frist-time definition overlaps the protocol of Metaclasses"
>                                 (self confirm: ((selector , ' is used in the existing class system.
>   Overriding it could cause serious problems.
>   Is this really what you want to do?') asText makeBoldFrom: 1 to: selector size))
>                                 ifFalse: [^nil]].
>         category := selectedMessageName
>                 ifNil: [ self selectedMessageCategoryName ]
>                 ifNotNil: [ (selectedClassOrMetaClass >> selectedMessageName) methodReference ifNotNil: [ : ref | ref category ]].
> +       selector := self
> +               basicCompile: aString
> +               in: selectedClassOrMetaClass
> +               classified: category
> +               notifying: aController.
> -       selector := selectedClassOrMetaClass
> -                               compile: aString
> -                               classified: category
> -                               notifying: aController.
>         selector == nil ifTrue: [^ nil].
>         contents := aString copy.
>
>         self changed: #messageCategoryList. "Because the 'as yet unclassified' might just appear."
>         self changed: #messageList. "Because we have code-dependent list formatting by now such as #isDeprecated."
>
>         selector ~~ selectedMessageName
>                 ifTrue:
>                         [category = ClassOrganizer nullCategory
>                                 ifTrue: [self changed: #classSelectionChanged.
>                                                 self changed: #classList.
>                                                 self messageCategoryListIndex: 1].
>                         self setClassOrganizer.  "In case organization not cached"
>                         (oldMessageList includes: selector)
>                                 ifFalse: [self changed: #messageList].
>                         self messageListIndex: (self messageList indexOf: selector)].
>         ^ selector!
>
> Item was added:
> + ----- Method: CodeHolder>>basicCompile:in:classified:notifying: (in category 'code pane') -----
> + basicCompile: aString in: aClassOrMetaClass classified: category notifying: requestor
> +
> +       | source |
> +       source := SystemBrowser acceptWithPrettyPrint
> +               ifTrue: [aClassOrMetaClass prettyPrinterClass
> +                       format: aString in: aClassOrMetaClass notifying: requestor]
> +               ifFalse: [aString].
> +       ^ aClassOrMetaClass
> +               compile: source
> +               classified: category
> +               notifying: requestor!
>
> Item was changed:
>   ----- Method: CodeHolder>>compileMessage:notifying: (in category 'code pane') -----
>   compileMessage: aString notifying: aController
>         "Compile the code that was accepted by the user, placing the compiled method into an appropriate message category.  Return true if the compilation succeeded, else false."
>
>         | selectedMessageName selector category selectedClassOrMetaClass |
>         selectedMessageName := self selectedMessageName.
>         selectedClassOrMetaClass := self selectedClassOrMetaClass.
>         contents := nil.
>         selector := (selectedClassOrMetaClass newParser parseSelector: aString).
>         (self metaClassIndicated
>                 and: [(selectedClassOrMetaClass includesSelector: selector) not
>                 and: [Metaclass isScarySelector: selector]])
>                 ifTrue: ["A frist-time definition overlaps the protocol of Metaclasses"
>                                 (self confirm: ((selector , ' is used in the existing class system.
>   Overriding it could cause serious problems.
>   Is this really what you want to do?') asText makeBoldFrom: 1 to: selector size))
>                                 ifFalse: [^nil]].
>         category := self selectedMessageCategoryName.
> +       selector := self
> +               basicCompile: aString
> +               in: selectedClassOrMetaClass
> +               classified: category
> +               notifying: aController.
> -       selector := selectedClassOrMetaClass
> -                               compile: aString
> -                               classified: category
> -                               notifying: aController.
>         selector == nil ifTrue: [^ nil].
>         contents := aString copy.
>         currentCompiledMethod := selectedClassOrMetaClass compiledMethodAt: selector.
>         ^ true!
>
> Item was changed:
>   ----- Method: DependencyBrowser>>defineMessageFrom:notifying: (in category 'contents') -----
>   defineMessageFrom: aString notifying: aController
>         "Compile the expressions in aString. Notify aController if a syntax error occurs. Install the compiled method in the selected class classified under  the currently selected message category name. Answer the selector obtained if compilation succeeds, nil otherwise."
>         | selectedMessageName selector category oldMessageList |
>         selectedMessageName := self selectedMessageName.
>         oldMessageList := self messageList.
>         contents := nil.
>         selector := (self selectedClassOrMetaClass newParser parseSelector: aString).
> +       selector := self
> +               basicCompile: aString
> +               in: self selectedClassOrMetaClass
> +               classified: (category := self selectedMessageCategoryName)
> +               notifying: aController.
> -       selector := self selectedClassOrMetaClass
> -                               compile: aString
> -                               classified: (category := self selectedMessageCategoryName)
> -                               notifying: aController.
>         selector == nil ifTrue: [^ false].
>         contents := aString copy.
>         ^ true
>   !
>
> Item was changed:
>   AppRegistry subclass: #SystemBrowser
>         instanceVariableNames: ''
> +       classVariableNames: 'AcceptWithPrettyPrint BrowseWithDragNDrop BrowseWithPrettyPrint'
> -       classVariableNames: 'BrowseWithDragNDrop BrowseWithPrettyPrint'
>         poolDictionaries: ''
>         category: 'Tools-Base'!
>
>   !SystemBrowser commentStamp: '<historical>' prior: 0!
>   This is the AppRegistry class for class browsing!
>
> Item was added:
> + ----- Method: SystemBrowser class>>acceptWithPrettyPrint (in category 'preferences') -----
> + acceptWithPrettyPrint
> +       <preference: 'Accept with pretty-print' category: 'browsing' description: 'If true, browsers will automatically pretty-print every method when you accept it.' type: #Boolean>
> +       ^ AcceptWithPrettyPrint ifNil: [false].!
>
> Item was added:
> + ----- Method: SystemBrowser class>>acceptWithPrettyPrint: (in category 'preferences') -----
> + acceptWithPrettyPrint: aBoolean
> +       AcceptWithPrettyPrint := aBoolean.!
>
>

Hi Marcel,

these are very good points for further and tighter integration of a pretty printer into the system (I'm mainly referring to customized and project-specific settings), and I'm sure that Tom and his team will consider at least some of them for PoppyPrint. :-) Nevertheless, I don't think that we should hesitate to integrate new features like this one into the Trunk as experimental features. Unless turned on by default, I don't see how this could harm anyone, but it allows certain users to configure their image to better match their individual preference, so we can give them more freedom. I think we should be more open to and supportive of new ideas, even if they do not perfectly fit together. Last but not least, isn't this one of the core ideas of trunk-based development? :-)

At the moment, the preference offers a working prototype that interested users can enable - when working on projects that do not enforce a different coding style - and benefit from additional convenience while writing their code. I'm liking this feature very much while developing my latest project. I'm also making good experiences with the analogous concept in the JavaScript world (VS Code "formatOnSave" + eslint) these days.

> Squeak Trunk should not do that [enforce such formatting]

I'm not requesting that here. At least not yet. This will enough stuff for a future discussion. :-)

> it should [...] rather be done at commit time (or code-review time)