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

TonelReader should offer a super simple API to load one single file!!!! #41

Open
Ducasse opened this issue Jul 10, 2018 · 7 comments
Open
Labels

Comments

@Ducasse
Copy link

Ducasse commented Jul 10, 2018

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

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

@Ducasse
Copy link
Author

Ducasse commented Jul 15, 2018

I should be super stupid.... I cannot make this reader working.
The API is plain bad.
Why I cannot simply say

TonelReader loadFileName: 'Comix/SeriePrinter.class.st'

I have a folder with my files and none of the expressions below work.

TonelReader
on: (FileSystem workingDirectory / 'Comix') asFileReference
fileName: 'SeriePrinter.class.st'

TonelReader
on: 'Comix' asFileReference
fileName: 'SeriePrinter.class.st'

@Ducasse
Copy link
Author

Ducasse commented Jul 15, 2018

This also raises uncomprehensible errors:

(TonelReader
on: 'Comix' asFileReference
fileName: 'SeriePrinter.class.st') loadDefinitions

Telling that the directory does not exist! Of course this is a file that I pass.
So to me this API REALLY sucks.

At the end I will have lost 3 hours and feel like shit and forced to do everything by hand.
Or hack the reader which I do not want to do.

@estebanlm
Copy link
Contributor

This is because you are trying to use it in a different way as it was meant for. TonelReader is a special kind of Monticello reader, hence it was meant to read packages, not classes.
And the API is inherited from Monticello.

In your case, if you do:

(TonelReader
    on: '.' asFileReference
    fileName: 'Comix') 
    loadDefinitions

it will work as expected (loading the definitions of the full "Comix" package).

This does not means we should not expand the API to allow simple file-in/file-out of classes (and even methods), but well... we need to implement it, since the original purpose of Tonel was not exactly that.

@Ducasse
Copy link
Author

Ducasse commented Jul 16, 2018

Ok I understand now.
You should think that Tonel will be in the future the default format for Pharo source
so it should be really easy to use. And you should add this to the class comment.

Tonel is realllllly important. I was waiting for it since 2002.

@Ducasse Ducasse closed this as completed Jul 16, 2018
@Ducasse Ducasse reopened this Jul 16, 2018
@Ducasse
Copy link
Author

Ducasse commented Jul 16, 2018

Esteban if fileName: was packageName: then it would be a lot clearer.

(TonelReader
onFolder: '.' asFileReference
package: 'Comix')
loadDefinitions

@estebanlm
Copy link
Contributor

yes, we should add that api (but not rename the other, since is needed for compatibility with monticello)

@Ducasse
Copy link
Author

Ducasse commented Jul 16, 2018

Ok even better. This MC API sucks :)

@estebanlm estebanlm added this to the 1.0.10 milestone Sep 4, 2018
@estebanlm estebanlm modified the milestones: 1.0.10, 1.0.11 Jan 15, 2019
@guillep guillep removed this from the 1.0.14 milestone May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants