Closed Bug 498915 Opened 11 years ago Closed 11 years ago

[silme] Simplify the API basing on the experience we have so far


(Mozilla Localizations :: Infrastructure, defect)

Not set


(Not tracked)



(Reporter: zbraniecki, Unassigned)





(1 file)

0.7 is a great moment to simplify the core pieces of API and freeze them for 1.0 release.

Main area of interest is what is in silme.core and silme.diff, see the URL for more details.
The first WIP.

Split into two dimensions.

1) Classes:

 - silme.core.Entity
 - silme.core.List
 - silme.core.Object
 - silme.core.Package
 - silme.diff.Entity
 - silme.diff.List
 - silme.diff.Object
 - silme.diff.Package

2) Methods:

Here I have much more problems. The best way would be to go like jQuery:

Entity.value() - gets value, Entity.value('foo') - sets value.

The only problem is that it adds one 'if' into every value set and get, which multiplied by 2000-3000 times (amount of time you fire it on iterating through the whole package) it hits the performance by 2-5%. :(

For methods I'd definitely go for get_pkg, add_pkg, add_obj, get_obj, add_list, get_list.

Another thing is if we want to have del_* instead of remove_*.
Why not use getters and setters for value()?

Quick feedback, I just gave up on loading browser.dtd into silme. Just couldn't get it done. Docs didn't help, help didn't help, and I obviously didn't find the right entry point :-(.
(In reply to comment #3)
> Why not use getters and setters for value()?

Silme supports multiply values per entity. It means that one entity can have two values depending on its locale. We support getters and setters in form:

entity['pl'] = 'Value in polish'
entity['de'] = 'Value in german'

print entity['de']

Because the locale code is optional, and we want to support setting simple values when its not needed, we support

entity.set_value('foo') and entity.get_value()

I would love to add entity.value as getter/setter but when I tried it it got into infinite loop with __getitem__ and __setitem__ .

I'll give it another try. I really want to have it and I agree that would be best.

> Quick feedback, I just gave up on loading browser.dtd into silme. Just couldn't
> get it done. Docs didn't help, help didn't help, and I obviously didn't find
> the right entry point :-(.

What part was missing from such code:

import silme.format

io_client ='file')
elist = io_client.get_entitylist('./path/to/file.dtd')


I get the feedback and I want to improve that, but it's very hard for an author to find missing points on the learning curve :(
Attached patch WIP1Splinter Review

*) entity.value = 'foo' works
*) entity['code'] = 'foo2' too
*) entity.set_value('foo3') too
*) silme.core.Entity()
*) silme.core.List()
*) silme.core.Object()

I was thinking about switching List to do sth. like 
l = silme.core.List()
l['entity_id1'] = entity
l['entity_id2'] = entity2
print l['entity_id1']

In other words to make its API identical to dict (except of several methods like get_locale_codes()). The upside of this is a shorter syntax, the downside is that it makes silme.core.Object and silme.core.List have very different API's.

Also, I still don't like silme.core.List and silme.core.Object names (cause the class name is List and Object), all other projects use descriptive names, which in our case were L10nObject and EntityList.
Pike, Stef, Adrian, what do you think?
Guys? I need the feedback soon. The window for API changes for 0.7 is getting narrow.
From an almost-end-user perspective:

Please don't use any of the 3 proposals from Cryptic names are hard to learn and read.

Please drop all "l10n", a simple "list", "object", "package" are good enough. I don't think I ever confused them with python's built-in lists and objects. For documentation, you can always explicitly specify the module: "takes silme.core.List as the argument". I liked the class names proposed in comment 1.
Stas: that's my direction, but I'm aware that using class names like Object or List is not the optimal thing to do (we recently switched from Object to Blob for the reason).

So I'd like to get a consensus among those who may raise objections after the release :)
I don't really like the idea of changing "EntityList" to "List". It raises the, already high entry point to Silme (lack of full documentation), because a "List" could be a list of everything, e.g. list of files, list of entity id's, etc. Without checking what this is, one can get confused.

In bigger applications, it also raises possible trouble when there'll be already a different "List" object type in use (easy to avoid through "import ... as", but one must first notice what's wrong...).

And also in reply to Stef's comment on the wiki: I'd also prefer to use common abbreviations like "pkg", "lst" or "obj" instead of full names. When there is no widely known abbreviation, then we should use the full name.
regarding "Object":
maybe it's a bit long, but I'd like "SilmeObject" or "SilmeObj" instead.

We just renamed "Object" to "Blob". Now renaming something different to "Object" is... confusing. Fortunately, we are still in an early development stage for such changes.
Amount of changes for Silme 0.7. We may want to incorporate some of those patterns for format/io in Silme 0.8.
silme.core.Entity.get_value() -> silme.core.Entity.value
silme.core.Entity.get_value('ab') -> silme.core.Entity['ab']
silme.core.Entity.set_value('foo','ab') -> silme.core.Entity['ab'] = 'foo'
silme.core.Entity.remove_value('ab') -> del silme.core.Entity['ab']
silme.core.Entity.remove_value() -> del silme.core.Entity.value
silme.core.EntityList.get_entity('id') -> silme.core.EntityList['id']

(in above cases all get_value, set_value, remove_value still work, but the new syntax should be more handy)

Other changes:

silme.core.L10nObject -> silme.core.Structure
silme.core.L10nPackage -> silme.core.Package
silme.diff.entitylist -> silme.diff.list
silme.diff.l10nobject -> silme.diff.structure
silme.diff.l10nobject.L10nObjectDiff -> silme.diff.StructureDiff
silme.diff.L10nPackageDiff -> silme.diff.PackageDiff
silme.diff.package.L10nPackageDiff -> silme.diff.PackageDiff
silme.diff.L10nPackageDiff -> silme.diff.PackageDiff
silme.core.EntityList.get_entity_ids -> silme.core.EntityList.ids
silme.diff.PackageDiff.get_package -> silme.diff.PackageDiff.package
silme.diff.PackageDiff.get_packages -> silme.diff.PackageDiff.packages
silme.diff.PackageDiff.get_object_type -> silme.diff.PackageDiff.structure_type
silme.diff.PackageDiff.get_package_type -> silme.diff.PackageDiff.package_type
silme.core.L10nPackage.packages -> silme.core.L10nPackage._packages
silme.core.L10nPackage.objects -> silme.core.L10nPackage._structures
silme.diff.PackageDiff.has_object -> silme.diff.PackageDiff.has_structure
silme.core.Package.has_object -> silme.core.Package.has_structure
silme.diff.PackageDiff.get_objects -> silme.diff.PackageDiff.structures
silme.core.Package.get_package -> silme.core.Package.package
silme.core.Package.get_packages -> silme.core.Package.packages
silme.core.Package.get_objects -> silme.core.Package.structures
silme.diff.EntityListDiff.get_entities -> silme.diff.EntityListDiff.entities
silme.core.Package.get_object -> silme.core.Package.structure
silme.core.EntityList.get_entities -> silme.core.EntityList.entities

In order to ease the transition for any applications using Silme, I introduced a compatibility package:

In order to make your script work in Silme 0.7 you have to add "import silme.compatibility.v05" at the beginning of your script.

Then, you should try to comment out lines in one by one and update your script to make it work with 0.7 natively. In 0.8 I'll try to make the compatibility package report debug informations about lines to be updated.

This closes the 0.7 API changes as 0.7 is getting released.

Thank you all for contribution to this and let's see how it feels to code with the new syntax and how we want to improve it in 0.8. I want 0.8 to be the last with any API changes, so if you have something on your plate, please, make sure it gets into 0.8. :)

I updated for 0.8 :)
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.