pharo-vcs / tonel Goto Github PK

Looking at initial test errors turns up a reference to OsPlatform ... haven't look for others.

The package MonticelloTonel-FileSystem is quasi-platform specific ... at least until GemStone starts using FileSystem, so there appears to be the need for another mechanism ... perhaps it makes sense to make a platform-specific method -- isolating the reference to OsPlatform in it's own platform-specific method and then putting that method in a platform-specific package. ... this approach would make the most sense unless there are a lot of platform-specific references ...

Anyway, @estebanlm I am game for whatever approach you want to use ...

I start noticed that instance and class side extensions with same selector are moving between commits.
The reason is that tonel writer sorts extension methods only by selector but it should also consider the class

the method change is lost, I will bring it back

see pharo-vcs/iceberg#1000

The current on:fileName: API is bad. We should be able to do

(TonelReader new filenamed: 'ffo.st') loadDefinitions.

I still don't have Tonel ported yet, so I don't have a working example to play with ... however I am curious if and where you guarantee UTF8 encoding of the file contents ... I didn't notice anything in the tests that deal with UTF8 encoded source

For FileTree I have found it necessary to put in validation checks that the method protocol for class extensions matches the package name .... human beings will be editing the files by hand and they will be cutting and pasting, so these kind of detail errors will happen ... the consequence of not validating is that the package structure will be immediately corrupted once the incorrect protocol names are loaded into the system ... same thing should probably go for regular classes where the method protocol needs to NOT be a package name ...

Class categories are the same ... the category of the class has to match the name of the package ...

A trait can use special slots.
Today only the slot names are exported.

We are writing a GUI for Exercism.io to submit solutions directly from Pharo to their web server,
so we need to generate Tonel files as strings without touching the file system.
Refactor to facilitate this.
[EDIT: I forgot to mention, I'll submit a PR for this soon]

The implication of this is that every time someone on windows saves a packages all of the involved classes will show up with a host of line ending changes (I imagine) which would not be a good thing ... the format really needs to avoid spurious diffs as much as possible

Up on travis it seems that when I am loading my brand new package structure the image is getting confused and it seems that it might be due to the fact that Tonel is already in the image ... now this is Pharo7.0 so there might be bugs, but I would have expected the undeclared global in that method to be resolved after the package that contained the class was loaded ...

Pharo6.1 has a similar set of warnings, but the test failures appear to be of a more reasonable variety ... maybe it's premature to include Pharo7.0 in the travis tests?

Should be really commented and type of arguments should be much more precise.
And sketch of an example should also be added.

Taken from pharo-vcs/iceberg#568

I encountered a problem regarding the import of tonel repositories: It throws the TonelParseError 'Can''t parse method body'. To reproduce, simply create a Package containing a single Class with a single method. Source of that method is: [$$]

The export of the method works fine and the files look okay, but the import crashes.

Note: it works when using [ $$ ] instead of [$$]

When there is a trait that uses another trait with class-side method, but uses exclusion or aliasing for that class-side method, the exclusion and/or alias is not saved into file.
Way to reproduce:

1. Create Trait named MyTrait
2. Go to class-side of that MyTrait
3. Create class-side method named foo in trait named MyTrait
4. Create Trait named MySecondTrait that uses MyTrait
5. Go to class-side side of that MySecondTrait and alias #bar->#foo
6. Create repository, add the package with those traits to the repository and commit it in Tonel format
7. Open the exported MySecondTrait.class.st file and see that the information about alias is gone

Tested in Pharo 6.1 (60546) with Iceberg 1.4.1

Only two tests are being run, but there are a number of other tests that could be inherited.

I think that shouldInheritSelectors needs to be implemented to return true on the class-side to run the tests ...

We can deal with filetree based history by replaying the commits.

Following tests are failed:

testWriteMethodDefinitionWithStrangeComment
	| writer def stream |
	writer := TonelWriter new.
	stream := String new writeStream.
	def := MCMethodDefinition
		className: #Object
		classIsMeta: false
		selector: #a:b:
		category: 'accessing'
		timeStamp: nil
		source:
			'a: "b:" arg1 b: arg2
	^ 42'.
	writer writeMethodDefinition: def on: stream.
	self
		assert: stream contents
		equals:
			('
{ #category : #accessing }
Object >> a: "b:" arg1 b: arg2 [
	^ 42
]
' withLineEndings: OSPlatform current lineEnding)

Problem with algorithm which extracts method name line from sources:

TonelWritter>>selectorIsComplete: keywords in: aString
	| start |
	start := 1.
	keywords
		do: [ :each |
			| index |
			index := aString
				findString: each
				startingAt: start
				caseSensitive: true.
			index = 0
				ifTrue: [ ^ false ].
			start := index + each size ].
	^ true

Just stepping through the parser as it reads meta data and I see that it is simply counting { and }, so it is vulnerable to either one of those characters embedded in the meta data itself (including symbols) ...

right now they are as extensions of metacello but this is a problem because tonel may be loaded after it (and in general is like that).

origin issue: https://pharo.fogbugz.com/f/cases/20659

When I accidentally encountered the code of TonelParser>>methodBody
a voice in my head told: it's too complex to be honest
So I decided to see how long it would take to attack...
Less than 5 minutes, see this test:

testMethodBodyWithSquareBracketInsideLiteralArray
    self 
        assertParse: '[ "What about this valid literal Array guys" ^#( [ ) ]'
        rule: #methodBody
        equals: ' "What about this valid literal Array guys" ^#( [ ) '.

If ever you manage to fix it, I have more for you

testMethodBodyWithCommentNextToDollarCharLiteral
    self 
        assertParse: '[^$$"what about this one]"]'
        rule: #methodBody
        equals: '^$$"what about this one]"'.

The package Moose-TestResources-KGB-P11FullReferee contains only the method extensions. The method TonelWriter>>#writeSnapshot: does following:

self writePackage: (snapshot definitions 
		detect: #isOrganizationDefinition
		ifFound: [ :each | each ]
		ifNone: [ self createDefaultOrganizationFrom: (snapshot definitions detect: #isClassDefinition) ]).

The snapshot does contain no organization nor class definition (it contains only 3 method definitions) so the code fails.

Would like to write a package out to a different directory name (one base on the tag) - this isn't easy due to write package not using a setter (if it used self packageDir: I could subclass and override this).

writePackage: aDefinition
	"I'm assuming first category is actually the package"
	packageDir := aDefinition categories first.

Could we use a setter - or let you specifify a name in some way.

This would be handy for non git related usages (e.g. exercism)

Is there a reason that this is not a Metadataless repo? I'm now in the process of porting Tonel to the base of GemStone and there is no Monticello implementation present (using Cypress1 now an Cypress2 eventually) so I won't able to write the packages out and preserve the monticello metadata ... if it is needed, I understand, but if it isn't I'd like to nuke the metadata now as it will be much easier for me:)

Following test fails:

TonelParserTest>>testMethodBodyWithDollarAndBracket
	self 
		assertParse: '[ [ $$] ]'
		rule: #methodBody
		equals: ' [ $$] '.

It shows problem with methods which return dollar character from block:

AClass>>someMethod
   true ifTrue: [^$$]

Notice that space between dollar and bracket works fine

seems that the end of line exporting has problems since some invisible chars appears on end of method

when exporting a class with traits, I have this output:

Class {
	#name : #BlPoint,
	#superclass : #Object,
	#traits : 'TBlDebug',
	#classTraits : 'TBlDebug classTrait',
	#category : 'Bloc-Basic-Math'
}

The classTraits section is redundant (and is actually not needed to rebuild the class). This section has sense when there are rules that modifies the trait composition, but otherwise is cleaner not including it.

We create the file in the package directory ... presumably to preserve the name of the package ... or are we using the name of the directory for the name of the package (appears to be doing that now) ... so the package.st is either a unnecessary appendage, or we need to read the package name from the file or .... is something else planned for that file?

During ESUG (2017, so more than a year) we agreed a format for Tonel files that is slightly different from the one existing now.
We agreed:

• Left side of ston declarations (keys) will go in symbols.
• Right side will NOT be symbols. They will be plain strings and any transformation into symbols needs to be solved in image.

This easies some parsing, etc.

I would change the following method

TonelWriter>>#toSTON: anObject
	^ String streamContents: [ :out | 
			(TonelSTONWriter on: out)
				newLine: self newLine;
				nextPut: anObject ]

Currently, you cannot load a project in OSX if the sources are on a shared drive or external drive because OSX generates hidden metadata files that tonel tries to load.

I'll do a PR to ignore those files.

because there is no class organisation

I found that classes with nil superclass are written with #nil symbol instead of simple nil.
Is it intentional? If yes then close issue.

older Pharo versions do not initialise variables for trait definition, so we have a DNU

I've just forked tonel to https://github.com/GsDevKit/tonel with the plan to port tonel to GemStone base image over the next few weeks. Since tonel is ulitimately intended to be a cross-platform file format I think a cross platform project would be preferred --- using travis to valid changes against ... a common package structure, etc. --- Presumably there is a fair amount of code that can be shared between platforms -- but I admit that I haven't delved too deep into the code to see just how portable it currently is...

If there is interest in making the pharo-vcs-tonel project cross platform then I will try to partition the packages/code while porting to the GemStone base image and to the GsDevKit project (upon which Seaside is based).

If there is no interest in hosting a cross-platform project, then I will undertake that task and break the forked relationship as there will be little in common other than a common starting point ...

In my porting work I am going to add more packages to Tonel to support the fact that the base image in GemStone doesn't use Monticello, so the packages will have to refactored ...

By not having the packages isolated in a subdirectory, the home directory of the project starts to get cluttered with .package directories and other directories that might be created ... I may want to include tode/gemstone directories with platform specific scripts ...

Pushing the packages down a level would relieve the clutter ... this is pretty much a standard practice AFAICT, so perhaps there is some other reason for not putting the packages into a subdirectory?

TonelWriter>>#createDefaultOrganizationFrom: should return an MCOrganization but currently it can return a string (a category name). The code should be:

createDefaultOrganizationFrom: aCollection
	"answers a default organisation for the cases where there are none"

	"simplest case, I answer the clas definition"
	snapshot definitions 
		detect: #isClassDefinition
		ifFound: [ :each | ^ MCOrganizationDefinition categories: { each category } ].
	
	^ self createDefaultOrganizationFromDefinition: (snapshot definitions
		detect: #isMethodDefinition
		ifNone: [ self error: 'cannot determine package name from empty snapshot' ])

But this migration script is really great! Thanks a lot, I will convert all my gitfiletree projects to Iceberg+Tonel. :-)

TonelWriter class>>#exportClass:on: is very inefficient. It does o snapshot of all the package before which may be very slow:

[
    String streamContents: [:s |TonelWriter exportClass: Object on: s ]
] timeToRun "0:00:00:04.785"

We could use more direct approach, something like

[
    writer := TonelWriter new.
    class := Object.
    definition := class asClassDefinition.
    snapshot := MCSnapshot fromDefinitions: {definition}, (class localMethods collect: [:each | each asRingDefinition asMCMethodDefinition ]), (class classSide localMethods collect: [:each | each asRingDefinition asMCMethodDefinition]).
    writer instVarNamed: #snapshot put: snapshot.
    String streamContents: [ :aStream |
        writer writeClassDefinition: definition on: aStream.
        writer writeClassSideMethodDefinitions: definition on: aStream.
        writer writeInstanceSideMethodDefinitions: definition on: aStream ]
] timeToRun " 0:00:00:00.11"

#writeClass: and #writePackage: aren't listed as private and yet they can't be used standalone as they internally call #writeClassSideMethodDefinitions: aClassDefinition on: aStream and it references a snapshop.

Would be useful to perform these operations - and have some tests for them.

we have a PR that changed a class category to #category : #'FuelPlatform-Pharo-Core-FuelPlatform-Pharo-Core. Original category name was FuelPlatform-Pharo-Core. In Tonel the class is placed in a right directory named FuelPlatform-Pharo-Core. But in the image that was bootstrapped from this PR, the class is in a strange state:

• the class is installed in the system
• there is no package named FuelPlatform-Pharo-Core
• the class definition is shown as:

    instanceVariableNames: ''
    classVariableNames: ''
    package: 'FuelPlatform-Pharo-Core-FuelPlatform-Pharo-Core'

• FLPharoPlatform package name >>> #'FuelPlatform-Pharo-Core'
• the package is not registered:
  (RPackageOrganizer default packages includes: FLPharoPlatform package) >>> false

After 20 min looking for it I still do not find it.
I'm stupid for sure.

Is there a good reason why Tonel uses platform-specific line ending?

TonelWriter>>newLine
	 ^ OSPlatform current lineEnding

It produces a lot of problems, and I don't see any advantage.

In the latest Pharo 7, I get an error while loading some files.

The file concerned is:

https://github.com/jecisc/QuickAccess/blob/264b98edb7d6c9ca0f9f86340aae37405e2869f5/src/BaselineOfQuickAccess/BaselineOfQuickAccess.class.st

The error is:

TonelParser>>type
TonelParser>>typeDef
TonelParser>>document
TonelParser>>start
TonelParser class>>parseStream:
[ :s | TonelParser parseStream: s ] in [ :each | each readStreamDo: [ :s | TonelParser parseStream: s ] ] in TonelReader>>loadDefinitions in Block: [ :s | TonelParser parseStream: s ]
[ aBlock value: stream ] in FileReference(AbstractFileReference)>>readStreamDo: in Block: [ aBlock value: stream ]
BlockClosure>>ensure:
FileReference(AbstractFileReference)>>readStreamDo:
FileSystemDirectoryEntry>>readStreamDo:
[ :each | each readStreamDo: [ :s | TonelParser parseStream: s ] ] in TonelReader>>loadDefinitions in Block: [ :each | each readStreamDo: [ :s | TonelParser pa...etc...
Array(SequenceableCollection)>>collect:
TonelReader>>loadDefinitions
TonelReader(MCVersionReader)>>definitions
TonelReader(MCVersionReader)>>snapshot
[ self snapshot ] in TonelReader(MCVersionReader)>>basicVersion in Block: [ self snapshot ]
MCVersion>>snapshot
[ :ea | 
ea canOptimizeLoading
	ifTrue: [ ea patch applyTo: loader ]
	ifFalse: [ loader updatePackage: ea package withSnapshot: ea snapshot ] ] in MCVersionLoader>>basicLoadWithNameLike: in Block: [ :ea | ...
OrderedCollection>>do:
MCVersionLoader>>basicLoadWithNameLike:
[ self basicLoadWithNameLike: aString ] in MCVersionLoader>>loadWithNameLike: in Block: [ self basicLoadWithNameLike: aString ]
[ returnValue := aBlock value ] in [ [ returnValue := aBlock value ]
	ensure: [ self announceLoadStop: aString ] ] in MCVersionLoader>>announceLoad:do: in Block: [ returnValue := aBlock value ]
BlockClosure>>ensure:
[ [ returnValue := aBlock value ]
	ensure: [ self announceLoadStop: aString ] ] in MCVersionLoader>>announceLoad:do: in Block: [ [ returnValue := aBlock value ]...
BlockClosure>>ensure:
MCVersionLoader>>announceLoad:do:
MCVersionLoader>>loadWithNameLike:
[ self ensurePackage: version package.
self loadWithNameLike: version info name ] in [ | version |
version := versions first.
[ self ensurePackage: version package.
self loadWithNameLike: version info name ] asJob
	title: 'Loading ' , version info name asString;
	run ] in MCVersionLoader>>load in Block: [ self ensurePackage: version package....
BlockClosure>>cull:
[ ^ block cull: self ] in [ self prepareForRunning.
CurrentJob value: self during: [ ^ block cull: self ] ] in Job>>run in Block: [ ^ block cull: self ]

When I try to debug I see that it check if "ass {" matches "Class". I suspect that the problem is here because this class does not have a comment. Maybe the recent Tonel generates "" when there is no comment, but since I did not write the previous file by hand I think at one time it generated nothing.

Maybe Tonel should manage this case to be retro compatible?

When you commit packages in Tonel format for Pharo 6.1 and 7 the output is not the same.

For example:
#category : #'Boardwalk-Tests-Javascript'
#category : 'Boardwalk-Tests-Javascript'

A version gets the category as a String and the other as a Symbol.

There was some confusion because Tonel uses STON to represent meta data and STON has a slightly different idea of literal (unquoted) Symbols than standard Pharo.

Since Tonel already subclasses STONWriter, the following change would improve the situation.

TonelSTONWriter>>#isSimpleSymbol: symbol
	"Customize STON to only consider very clean symbols as literal, 
	for all others err on the safe side and quote them."
	
	symbol isEmpty
		ifTrue: [ ^ false ].
	('abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' includes: symbol first)
		ifFalse: [ ^ false ].
	^ symbol allSatisfy: [ :each | 
		'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789' includes: each ]

Fixed in #21