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

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

Open
gcotelli opened this issue Oct 19, 2021 · 10 comments
Open

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

gcotelli opened this issue Oct 19, 2021 · 10 comments

Comments

@gcotelli
Copy link

gcotelli commented Oct 19, 2021

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

It looks like pharo-vcs/tonel#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
Copy link
Contributor

dalehenrich commented Oct 19, 2021

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
Copy link
Collaborator

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 pharo-vcs/tonel#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
Copy link
Author

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
Copy link
Collaborator

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

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

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
Copy link
Collaborator

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

dalehenrich commented Oct 20, 2021

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

I've created a PR to fix the issue in Pharo pharo-vcs/tonel#100

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

3 participants