instantiations / tonel-vast

Tonel file format writer and reader implementation for VA Smalltalk
MIT License
9 stars 3 forks source link

Class names and categories are exported as strings and not symbols #102

Open gcotelli opened 3 years ago

gcotelli commented 3 years ago

At least in Pharo 9, Tonel generated files use symbols for class names and method categories, but the TonelWriter in VAST uses strings instead. It would be good to reach an agreement so exporting code from VAST would not generate differences.

For example:

Class {
-   #name : #TestAsserterBuoyExtensionsTest,
-   #superclass : #TestCase,
-   #category : #'Buoy-SUnit-Tests'
+   #name : 'TestAsserterBuoyExtensionsTest',
+   #superclass : 'TestCase',
+   #category : 'Buoy-SUnit-Tests'
 }

-{ #category : #tests }
+{ #category : 'tests' }
 TestAsserterBuoyExtensionsTest >> testAssertIncludes [

    | asserter |

    asserter := TestAsserter new.
    self shouldnt: [ asserter assert: #(1) includes: 1 ] raise: TestFailure.
    self should: [ asserter assert: #() includes: 1 ] raise: TestFailure.
    self should: [ asserter assert: #(1 2) includes: 3 ] raise: TestFailure
 ]
gcotelli commented 3 years ago

It looks like https://github.com/pharo-vcs/tonel/issues/52 is related. I don't know what is the best option, but for sure an agreement between the several vendors implementing the format is needed so people wanting to produce cross-platform code can at least rely on the format being the same.

dalehenrich commented 3 years ago

The spec for tonel that was agreed upon at least 3 years ago, was that all keys would be Symbols and all values would be Strings and that is what pharo-vcs/tonel#52 records ...

Over the years I have seen spotty conformance to that spec in the pharo code repositories and it seems that Pharo itself writes packages out with random conformance to the spec.

In self defense, I'm planning to add a pharo-tonel mode to Rowan to attempt to preserve the original format of the repository (until the pharo folks themselves settle on using a consistent format ...

eMaringolo commented 3 years ago

It would be really simple to change one for another, in fact we moved from Symbols to Strings when serializing, because we found differences when porting some projects which when exported back from VAST caused code differences only because of identifiers.

Also, as @dalehenrich points out, our current implementation follows the https://github.com/pharo-vcs/tonel/issues/52 agreement: keys are Symbols, values are Strings.

We have no hard position on this, since the Reader and Loader is considering that the input might be mixed, and coerces the types into what we internally need.

gcotelli commented 3 years ago

Maybe the TonelWriter can be made configurable, so we can export the code to match the convention used in the project until all the vendors implement the agreed convention.

eMaringolo commented 3 years ago

Maybe the TonelWriter can be made configurable, so we can export the code to match the convention used in the project until all the vendors implement the agreed convention.

That would be the most flexible approach.

dalehenrich commented 3 years ago

Of course EVERYONE would have to follow that convention as well ... one vendor not playing along will still cause problems ...The BEST SOLUTION is for everyone to actually use the AGREED UPON format ...

dalehenrich commented 3 years ago

I just noticed that Pharo (randomly?) writes mixed Strings and Symbols as values ...this was written 8 days ago and this was written 19 days ago ... hint look at the category field ... so I plan to abandon the idea of trying create a pharo_tonel mode in Rowan, since pharo is not even self consistent ...

eMaringolo commented 3 years ago

I just noticed that Pharo (randomly?) writes mixed Strings and Symbols as values ...this was written 8 days ago and this was written 19 days ago ... hint look at the category field ... so I plan to abandon the idea of trying create a pharo_tonel mode in Rowan, since pharo is not even self consistent ...

This I noticed too some time ago. I don't know if it has to do with some version of Pharo (8 vs 9?) or what.

dalehenrich commented 3 years ago

I don't know if it has to do with some version of Pharo (8 vs 9?) or what.

agreed, but if it is indeed "fixed in Pharo9" (I've also observed that a number of projects wrote things correctly back in July), then that is another argument to just write things correctly without a special pharo_tonel format and expect that moving forward, we'll converge on the correct format ...

gcotelli commented 3 years ago

I've created a PR to fix the issue in Pharo https://github.com/pharo-vcs/tonel/pull/100