Open
Bug 455783
Opened 16 years ago
Updated 4 years ago
[silme] support PEP08 and PEP257 guidelines
Categories
(Localization Infrastructure and Tools :: Silme, defect)
Localization Infrastructure and Tools
Silme
Tracking
(Not tracked)
NEW
People
(Reporter: zbraniecki, Unassigned)
References
()
Details
Attachments
(2 files, 4 obsolete files)
We're currently using a coding style mostly based on Java one. It's the one I'm most familiar and friendly with. Unfortunately, python not only has its own guideline which is very different, but its also very strict about it. In other words if we think about using parts of silme as a reference or base for others code (like apps basing on silme made by people outside of Mozilla) and/or making parts of silme travel around the world (like, l20n parser/structure/processor) it seems to be highly expected to follow PEP08. PEP08 is a basic guideline for python and from our standpoint requires two things: 1) Using lower case names of functions and variables 2) Using 4-spaces indent Such a change will be new to me, and I think to all of us, but I believe it's mostly a matter of getting used to it. Let me summarize what I have on it: Pros: * Following standards * Making the code more readable for outsiders * Making the code more ready to go outside of us for use of third-party companies, volunteers, making silme a better participant of the open source environment * Raises the chances of L20n parser to be used by third party Cons: * It breaks the API of Silme * 4-spaces may make it harder to keep 69 chars lines especially with descriptive functions names. * It introduces a new style of writing that we haven't use before After initial strong opposition inside my mind, I decided to follow the PEP08 for the same reason I would follow kernel guidelines, java guidelines or mozilla guidelines when writing in those environments. I want to minimize the cons so I'm introducing a python script that maps new function names to the old ones in silme.playground.compatible and I'm delaying 4-indent switch until better moment.
Reporter | ||
Comment 1•16 years ago
|
||
please, share your opinions on this. I think its important issue to solve before we freeze API
Attachment #339130 -
Flags: review?
Reporter | ||
Comment 2•16 years ago
|
||
with this patch, if you want the old code to work, you just have to add "import silme.playground.compatible" at the top of your script. Done
Reporter | ||
Comment 3•16 years ago
|
||
updated patch to follow the name() pattern (PEP08)
Attachment #339130 -
Attachment is obsolete: true
Attachment #339130 -
Flags: review?
Comment 4•16 years ago
|
||
Don't know how many people use current trunk but it would be useful (at least for me) to land this separately from multilocale branch merge. As you mention above, this is different, foreign coding style form general mozilla one - my filling is that making the code perfectly compliant with the PEP08 rules will make it look strange for mozdevs with unknown wins in adoption by different projects topic; e.g. do we know at least one project that strictly require full compliance before external library incorporation?
Reporter | ||
Comment 5•16 years ago
|
||
(In reply to comment #4) > Don't know how many people use current trunk but it would be useful (at least > for me) to land this separately from multilocale branch merge. It would require me to finish changes in multilocale branch using old notation. Is there a problem for you to use silme.playground.compatible ? > As you mention above, this is different, foreign coding style form general > mozilla one - my filling is that making the code perfectly compliant with the > PEP08 rules will make it look strange for mozdevs with unknown wins in adoption That's the notation you see in the patch. Is that that strange for you? > by different projects topic; e.g. do we know at least one project that strictly > require full compliance before external library incorporation? My experience with getting feedback on silme from python devs is very much unified. They all require it, but just for example - TranslateToolkit and Pootle.
Comment 6•16 years ago
|
||
(In reply to comment #5) > (In reply to comment #4) >> Don't know how many people use current trunk but it would be useful (at >> least for me) to land this separately from multilocale branch merge. > > It would require me to finish changes in multilocale branch using old notation. No, not necessarily - but I understand that this is additional work from your point of view. > Is there a problem for you to use silme.playground.compatible ? Not a problem, but I just don't like atomic commits and that would also mean for me, that current trunk is completely dead - nothing more, I don't plan to use compatibility module anyway. > > As you mention above, this is different, foreign coding style form general > > mozilla one - my filling is that making the code perfectly compliant with the > > PEP08 rules will make it look strange for mozdevs with unknown wins in adoption > > That's the notation you see in the patch. Is that that strange for you? Maybe the problem is that I don't accept things just because they are; it's a personal preference, but yes: four spaces indents and names not in camel case, underscores everywhere (probably because of hungarian notation) are weird to me. > > by different projects topic; e.g. do we know at least one project that strictly > > require full compliance before external library incorporation? > > My experience with getting feedback on silme from python devs is very much > unified. They all require it, but just for example - TranslateToolkit and > Pootle. My preferences, like mentioned above, simply don't count if it is so important for them to require this.
Reporter | ||
Comment 7•16 years ago
|
||
One of the elements of this move should be to change the name "object" to something else. So: 1) object.py will change name 2) Object class will change name I don't have any idea what should be the new names :((
Reporter | ||
Comment 8•16 years ago
|
||
http://svn.browsershots.org/trunk/devtools/pep8/pep8.py - script that verifies pep8 compilance
Reporter | ||
Comment 9•16 years ago
|
||
I started working on silme.core.entity wrt this and I'm also applying PEP 257 here.
Summary: [silme] support PEP08 guidelines → [silme] support PEP08 and PEP257 guidelines
Reporter | ||
Comment 10•16 years ago
|
||
This is first patch from WIP on PEP08/PEP257. It's made just for the sake of release-early-&-often principle. You can review it, or you may want to wait for the final version with documentation and unit tests. It has no docstrings yet, but it has everything I wanted to include in the release for silme.core.entity.Entity class. (I didn't work on EntityList yet). It does break the whole lib, since it changes the API. It does support several nice things like: 1) If you print entity it will print a beautiful entity info instead of object reference 2) You can use entity['pl'] = 'value' 3) You can del entity['de'] 4) You can print entity['pl'] 5) or you can print entity.value (default value) It shoudl give us a nice blend between two major different use cases: a) When someone is operating on files that have only one value per key (DTD, properties) and he doesn't have/doesn't want to care about different language codes in entities he can simply use Entity('id','value') and then entity.value. b) When someone is operating on multi value entities like PO/XLIFF where he might want to do entity['pl']='x', entity['de']='f', print entity['pl'] It also nicely falls back with default_code. I'm beginning working on docstrings and unit tests for silme.core.entity.Entity now. Feedback welcomed :)
Attachment #339143 -
Attachment is obsolete: true
Reporter | ||
Comment 11•16 years ago
|
||
slightly updates version. It can print entity without value, it has all lines below 80chars and it uses beauty string building pythonization instead of boring concatenation.
Attachment #350536 -
Attachment is obsolete: true
Reporter | ||
Comment 12•16 years ago
|
||
WIP v3 - with get_default_code method.
Attachment #350540 -
Attachment is obsolete: true
Reporter | ||
Comment 13•16 years ago
|
||
example unittest WIP
Reporter | ||
Comment 14•16 years ago
|
||
First part landed. silme.core.entity.Entity and EntityList is PEP08 compliant. There are also unit tests for some of the features. Next step is to do that for silme.core.object
Reporter | ||
Comment 15•16 years ago
|
||
The codebase has been PEP08ed!!! :) I'll be using pythontidy to look for more cleanups, but they should not touch the API. The only issue left in this bug is case of long variables/function names. Example: read_l10npackagediff_from_text() instead of: readL10nPackageDiffFromText() looks hardly readable for me. Will try to shortcut where possible.
Reporter | ||
Comment 16•15 years ago
|
||
as in bug 466594, this one will stay open for 0.5 release. A lot has been done, the library is clean PEP08 right now and core/diff follow PEP257 too. We'll be working on that after 0.5.
Comment 17•6 years ago
|
||
Moving Silme bugs to its component. [Mass change filter: silme-move]
Component: Infrastructure → Silme
Product: Mozilla Localizations → Localization Infrastructure and Tools
Reporter | ||
Updated•4 years ago
|
Assignee: gandalf → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•