[squeak-dev] Review Request: stylePackageScripts.2.cs

christoph.thiede at student.hpi.uni-potsdam.de christoph.thiede at student.hpi.uni-potsdam.de
Mon Nov 22 15:35:22 UTC 2021


Hi Marcel,

thanks for the fast feedback!

> Please double-check to not upload texts (or their serialized form) to the server.

Good catch! ["stylePackageScripts.3.cs"] resolves this by converting the script to a string prior to serialization (see MCStWriter >> #writeScriptDefinition:). Updated diff is below. Addressing this issue in the writer seems to be the safest way to me, and you will not loose your formatting prior to exporting the package, which aligns to the behavior of class comments and method comments.

(However, I would actually love to finally be able to serialize formatted comments via Monticello - but that should be stuff for its own debate.)

So can I go ahead and merge this? :-)

Best,
Christoph


=============== Summary ===============

Change Set:		stylePackageScripts
Date:			22 November 2021
Author:			Christoph Thiede

This change set enables syntax highlighting for package scripts ({preamble,postscript}{, of removal}). To achieve this, the instance variables in PackageInfo are converted to hold plain strings or texts rather than StringHolders, and the client (Monticello) is reponsible itself for displaying them to the user and storing them back into the package. Note the postscript which converts all PackageInfo instances in the image.

=============== Postscript ===============

"Postscript:
Eliminate StringHolder script in PackageInfos"
PackageInfo allSubInstancesDo: [:packageInfo |
	#(preamble postscript preambleOfRemoval postscriptOfRemoval) do: [:sel |
		(packageInfo instVarNamed: sel) ifNotNil: [:stringHolder |
			packageInfo instVarNamed: sel put: stringHolder contents]]].

=============== Diff ===============

MCStWriter>>writeScriptDefinition: {writing} · ct 11/22/2021 16:34 (changed)
writeScriptDefinition: definition
	stream nextChunkPut: (
		'(PackageInfo named: {1}) {2}: {3}'
		format: {
			"{1}" definition packageName printString.
			"{2}" definition scriptSelector. 
- 			"{3}" definition script printString
+ 			"{3}" definition script asString printString
		}); cr

MCWorkingCopyBrowser>>editLoadScripts {morphic ui} · ct 11/22/2021 16:05 (changed)
editLoadScripts

	| arg |
	self hasWorkingCopy ifFalse: [^self].
	arg := UIManager default
- 		chooseFrom: #('edit preamble' 'edit postscript' 'edit preambleOfRemoval' 'edit postscriptOfRemoval')
+ 		chooseFrom: {'edit preamble' translated. 'edit postscript' translated. 'edit preambleOfRemoval' translated. 'edit postscriptOfRemoval' translated}
		values: #(#preamble #postscript #preambleOfRemoval #postscriptOfRemoval).

	arg ifNotNil: [
		self editScript: arg].

MCWorkingCopyBrowser>>editScript: {morphic ui} · ct 11/22/2021 15:59 (changed)
editScript: scriptSymbol

- | script |
- script := workingCopy packageInfo perform: scriptSymbol.
- script openLabel: scriptSymbol asString, ' of the Package ', workingCopy package name.
+ 	| script |
+ 	script := workingCopy packageInfo perform: scriptSymbol.
+ 	Project uiManager
+ 		edit: script
+ 		label: ('{1} of the Package {2}' translated format: {scriptSymbol asString capitalized. workingCopy package name})
+ 		shouldStyle: true
+ 		accept: [:result | workingCopy packageInfo
+ 			perform: scriptSymbol asSimpleSetter
+ 			with: result]

PackageInfo>>isScript:not: {preamble/postscript} · ct 11/22/2021 15:53 (changed)
isScript: script not: default
	^ script notNil
		and: [ | contents |
- 			contents := script contents asString withBlanksTrimmed.
+ 			contents := script asString withBlanksTrimmed.
			contents notEmpty and: [contents ~= default and: [contents ~= 'nil']]]

PackageInfo>>postscript {preamble/postscript} · ct 11/22/2021 15:53 (changed)
postscript
+ 
	^ postscript ifNil: [
- 		postscript := StringHolder new contents: self postscriptDefault]
+ 		postscript := self postscriptDefault]

PackageInfo>>postscript: {preamble/postscript} · ct 11/22/2021 15:57 (changed)
postscript: aString

- postscript := StringHolder new contents: aString
+ 	postscript := aString

PackageInfo>>postscriptOfRemoval {preamble/postscript} · ct 11/22/2021 15:53 (changed)
postscriptOfRemoval
	^ postscriptOfRemoval ifNil: [
- 		postscriptOfRemoval := StringHolder new contents: self postscriptOfRemovalDefault]
+ 		postscriptOfRemoval := self postscriptOfRemovalDefault]

PackageInfo>>postscriptOfRemoval: {preamble/postscript} · ct 11/22/2021 15:57 (changed)
postscriptOfRemoval: aString

- postscriptOfRemoval := StringHolder new contents: aString
+ 	postscriptOfRemoval := aString


PackageInfo>>preamble {preamble/postscript} · ct 11/22/2021 15:54 (changed)
preamble
	^ preamble ifNil: [
- 		preamble := StringHolder new contents: self preambleDefault]
+ 		preamble := self preambleDefault]

PackageInfo>>preamble: {preamble/postscript} · ct 11/22/2021 15:56 (changed)
preamble: aString

- preamble := StringHolder new contents: aString
+ 	preamble := aString

PackageInfo>>preambleOfRemoval {preamble/postscript} · ct 11/22/2021 15:54 (changed)
preambleOfRemoval
	^ preambleOfRemoval ifNil: [
- 		preambleOfRemoval := StringHolder new contents: self preambleOfRemovalDefault]
+ 		preambleOfRemoval := self preambleOfRemovalDefault]

PackageInfo>>preambleOfRemoval: {preamble/postscript} · ct 11/22/2021 15:56 (changed)
preambleOfRemoval: aString

- preambleOfRemoval := StringHolder new contents: aString
+ 	preambleOfRemoval := aString

---
Sent from Squeak Inbox Talk

On 2021-11-22T16:22:02+01:00, marcel.taeumel at hpi.de wrote:

> So, preamble and postscript would now be regular Strings instead? Sounds good to me, given that they are stored as strings on the server anyway.
> 
> Please double-check to not upload texts (or their serialized form) to the server.
> 
> Best,
> Marcel
> Am 22.11.2021 16:17:43 schrieb christoph.thiede at student.hpi.uni-potsdam.de <christoph.thiede at student.hpi.uni-potsdam.de>:
> Please give me a short feedback on whether you agree with the idea of this change (PackageInfo scripts are no longer instances of StringHolder). If there is no objection, I will try to merge this into the Trunk after a week or so. :-)
> 
> ["stylePackageScripts.png"]
> 
> Best,
> Christoph
> 
> 
> =============== Summary ===============
> 
> Change Set:        stylePackageScripts
> Date:            22 November 2021
> Author:            Christoph Thiede
> 
> This change set enables syntax highlighting for package scripts ({preamble,postscript}{, of removal}). To achieve this, the instance variables in PackageInfo are converted to hold plain strings or texts rather than StringHolders, and the client (Monticello) is reponsible itself for displaying them to the user and storing them back into the package. Note the postscript which converts all PackageInfo instances in the image.
> 
> =============== Postscript ===============
> 
> "Postscript:
> Eliminate StringHolder script in PackageInfos"
> PackageInfo allSubInstancesDo: [:packageInfo |
>     #(preamble postscript preambleOfRemoval postscriptOfRemoval) do: [:sel |
>         (packageInfo instVarNamed: sel) ifNotNil: [:stringHolder |
>             packageInfo instVarNamed: sel put: stringHolder contents]]].
> 
> =============== Diff ===============
> 
> MCWorkingCopyBrowser>>editLoadScripts {morphic ui} · ct 11/22/2021 16:05 (changed)
> editLoadScripts
> 
>     | arg |
>     self hasWorkingCopy ifFalse: [^self].
>     arg := UIManager default
> -         chooseFrom: #('edit preamble' 'edit postscript' 'edit preambleOfRemoval' 'edit postscriptOfRemoval')
> +         chooseFrom: {'edit preamble' translated. 'edit postscript' translated. 'edit preambleOfRemoval' translated. 'edit postscriptOfRemoval' translated}
>         values: #(#preamble #postscript #preambleOfRemoval #postscriptOfRemoval).
> 
>     arg ifNotNil: [
>         self editScript: arg].
> 
> MCWorkingCopyBrowser>>editScript: {morphic ui} · ct 11/22/2021 15:59 (changed)
> editScript: scriptSymbol
> 
> - | script |
> - script := workingCopy packageInfo perform: scriptSymbol.
> - script openLabel: scriptSymbol asString, ' of the Package ', workingCopy package name.
> +     | script |
> +     script := workingCopy packageInfo perform: scriptSymbol.
> +     Project uiManager
> +         edit: script
> +         label: ('{1} of the Package {2}' translated format: {scriptSymbol asString capitalized. workingCopy package name})
> +         shouldStyle: true
> +         accept: [:result | workingCopy packageInfo
> +             perform: scriptSymbol asSimpleSetter
> +             with: result]
> 
> PackageInfo>>isScript:not: {preamble/postscript} · ct 11/22/2021 15:53 (changed)
> isScript: script not: default
>     ^ script notNil
>         and: [ | contents |
> -             contents := script contents asString withBlanksTrimmed.
> +             contents := script asString withBlanksTrimmed.
>             contents notEmpty and: [contents ~= default and: [contents ~= 'nil']]]
> 
> PackageInfo>>postscript {preamble/postscript} · ct 11/22/2021 15:53 (changed)
> postscript
> +
>     ^ postscript ifNil: [
> -         postscript := StringHolder new contents: self postscriptDefault]
> +         postscript := self postscriptDefault]
> 
> PackageInfo>>postscript: {preamble/postscript} · ct 11/22/2021 15:57 (changed)
> postscript: aString
> 
> - postscript := StringHolder new contents: aString
> +     postscript := aString
> 
> PackageInfo>>postscriptOfRemoval {preamble/postscript} · ct 11/22/2021 15:53 (changed)
> postscriptOfRemoval
>     ^ postscriptOfRemoval ifNil: [
> -         postscriptOfRemoval := StringHolder new contents: self postscriptOfRemovalDefault]
> +         postscriptOfRemoval := self postscriptOfRemovalDefault]
> 
> PackageInfo>>postscriptOfRemoval: {preamble/postscript} · ct 11/22/2021 15:57 (changed)
> postscriptOfRemoval: aString
> 
> - postscriptOfRemoval := StringHolder new contents: aString
> +     postscriptOfRemoval := aString
> 
> 
> PackageInfo>>preamble {preamble/postscript} · ct 11/22/2021 15:54 (changed)
> preamble
>     ^ preamble ifNil: [
> -         preamble := StringHolder new contents: self preambleDefault]
> +         preamble := self preambleDefault]
> 
> PackageInfo>>preamble: {preamble/postscript} · ct 11/22/2021 15:56 (changed)
> preamble: aString
> 
> - preamble := StringHolder new contents: aString
> +     preamble := aString
> 
> PackageInfo>>preambleOfRemoval {preamble/postscript} · ct 11/22/2021 15:54 (changed)
> preambleOfRemoval
>     ^ preambleOfRemoval ifNil: [
> -         preambleOfRemoval := StringHolder new contents: self preambleOfRemovalDefault]
> +         preambleOfRemoval := self preambleOfRemovalDefault]
> 
> PackageInfo>>preambleOfRemoval: {preamble/postscript} · ct 11/22/2021 15:56 (changed)
> preambleOfRemoval: aString
> 
> - preambleOfRemoval := StringHolder new contents: aString
> +     preambleOfRemoval := aString
> ["stylePackageScripts.2.cs"]
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20211122/f05bc9a3/attachment.html>
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20211122/8b2a60f1/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stylePackageScripts.3.cs
Type: application/octet-stream
Size: 3612 bytes
Desc: not available
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20211122/8b2a60f1/attachment-0001.obj>


More information about the Squeak-dev mailing list