Metacello / metacello

Metacello is a package management system for Smalltalk
MIT License
87 stars 43 forks source link

Monticello FileTree: Cannot store & load methods with a backslash in the selector #554

Closed LinqLover closed 1 year ago

LinqLover commented 1 year ago

Methods with a backslash in the selector (such as Number>>#\\) are stored in the methodProperties.json without any escaping. For selectors such as \\, this defect is masked because the key is interpreted as just \, but for other selectors like \%, this even leads to a json parsing error during loading. I'm pretty sure that there is missing some proper escaping.

dalehenrich commented 1 year ago

@LinqLover, there are several methods in the FileTree code base name #fileNameForSelector: (https://github.com/dalehenrich/filetree/blob/gemstone2.4/repository/MonticelloFileTree-Core.package/MCFileTreeAbstractStWriter.class/instance/fileNameForSelector..st, https://github.com/dalehenrich/filetree/blob/gemstone2.4/repository/MonticelloFileTree-Core.package/MCFileTreeStCypressWriter.class/instance/fileNameForSelector..st, and https://github.com/dalehenrich/filetree/blob/gemstone2.4/repository/MonticelloFileTree-Core.package/MCFileTreeStCypressWriter.class/class/fileNameForSelector..st) that is expected to do the translation, but I don't know if you guys use that project for FileTree, since the Squeak4.3 branch was last touched 8 years ago) ...the gemstone2.4 branch was last touched 2 years ago and is still actively used today ...it looks like the three methods date from about 8 years ago for GemStone so you might just be sharing the same code ???

LinqLover commented 1 year ago

@dalehenrich Yes, I have these selectors (for instance MCFileTreeStCypressWriter class>>#fileNameForSelector: dkh 9/8/2013 07:16:04). But I think the issue here is not that some character is not translated to a special word ("backslash"), but that there is no escaping when storing the selectors in the JSON file.

The issue might be located here:

String>>writeCypressJsonOn: aStream forHtml: forHtml indent: startIndent
    "by default ignore <forHtml> ... <forHtml> is used for Dictionary and Array, i.e., container objects and String which actually encodes itself differently for HTML"

    aStream
        nextPutAll: '"';
        nextPutAll:
                (forHtml
                        ifTrue: [ self cypressEscape ]
                        ifFalse: [ self ]);
        nextPutAll: '"'

Maybe we need #cypressEscapeForHTML and #cypressEscape(ForJSON) too? What would be the right branch to create this issue against, would it be this one? https://github.com/dalehenrich/filetree/blob/squeak4.3/repository/MonticelloFileTree-Core.package/String.extension/instance/writeCypressJsonOn.forHtml.indent..st

LinqLover commented 1 year ago

By the way, could it be time to fork a new squeak6.0 branch from squeak4.3? :)

dalehenrich commented 1 year ago

@LinqLover I've invited you to be a contributor to FileTree and it isn't clear to me what you can and cannot do as a contributor, but if you can add a branch then feel free to add it, otherwise I'll be glad to add it for you ... you could also consider creating a Squeak fork of the project and have full control :smile:

LinqLover commented 1 year ago

Thank you 🙏 I was asking because I'd suppose that over time working with one branch only could make compatibility handling unnecessarily hard. I don't think that we need a fork, though.

If we want to create a new branch, the references in https://github.com/Metacello/metacello/blob/master/repository/BaselineOfMetacello.package/BaselineOfMetacello.class/instance/baseline..st#L271 and https://github.com/hpi-swa/Squot/blob/develop/src/BaselineOfSquot.package/BaselineOfSquot.class/instance/baseline..st#L10 will need to be updated depending on the current Squeak version.

My suggestion would be that we should create a new squeak6.0 branch from the current squeak4.3 the next time we need to make any change to FileTree that depends on a newer Squeak version and update the references I mentioned above. How does that sound @krono @j4yk? :-)

j4yk commented 1 year ago

@LinqLover As you can see, maintaining different target platforms on different branches and keeping things up-to-date, yet working, is not so easy in general. Adding yet another branch will not solve that problem. However, staying with that model I would prefer to avoid new API or behavior that changed between platform versions as much as possible, and stay with the existing branches. When it becomes absolutely necessary because of some breaking change, you can still establish a new branch for the later Squeak version. Or has the moment already come?

LinqLover commented 1 year ago

@j4yk No, that moment has not yet come. :D Fair points, so let's try to stick with the existing branch for as long as we can.