Closed Bug 498915 Opened 11 years ago Closed 11 years ago
[silme] Simplify the API basing on the experience we have so far
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%. :( Comments?
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_*.
Status: NEW → ASSIGNED
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 import silme.io silme.format.Manager.register('dtd') io_client = silme.io.Manager.get('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 :(
WIP1 *) 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 https://wiki.mozilla.org/Silme:Naming_convention. 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: http://hg.mozilla.org/users/zbraniecki_mozilla.com/silme/file/tip/lib/silme/compatibility/v05.py 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 v05.py 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 https://wiki.mozilla.org/Silme:Naming_convention for 0.8 :)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.