Open Bug 455783 Opened 12 years ago Updated 2 years ago

[silme] support PEP08 and PEP257 guidelines

Categories

(Localization Infrastructure and Tools :: Silme, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: zbraniecki, Assigned: zbraniecki)

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.
please, share your opinions on this. I think its important issue to solve before we freeze API
Attachment #339130 - Flags: review?
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
updated patch to follow the name() pattern (PEP08)
Attachment #339130 - Attachment is obsolete: true
Attachment #339130 - Flags: review?
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?
(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.
(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.
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 :((
Blocks: 458441
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
Attached patch silme.core.entity cleanup WIP (obsolete) — Splinter Review
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
Attached file silme.core.entity cleanup WIP v2 (obsolete) —
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
WIP v3 - with get_default_code method.
Attachment #350540 - Attachment is obsolete: true
Depends on: 465861
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
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.
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.
Blocks: 478225
Moving Silme bugs to its component.

[Mass change filter: silme-move]
Component: Infrastructure → Silme
Product: Mozilla Localizations → Localization Infrastructure and Tools
You need to log in before you can comment on or make changes to this bug.