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

Feature/transparent declared for python2 #343

Merged
merged 12 commits into from Apr 13, 2014

Conversation

Projects
None yet
2 participants
@DXist
Copy link
Contributor

commented Mar 17, 2014

This is an attempt to implement ordered class attributes for Python 2.

See #313 for solution that requires class customization.

For statically defined classes there is inspect magic, that tries to find class code object.

Those users who create classes dynamically have to specify class attributes as odict instance.

@plq

This comment has been minimized.

Copy link

commented on spyne/model/complex.py in 5f18187 Mar 18, 2014

don't we use the metaclass for both python 3 and 2 but in 3 it's just a noop?

@plq

This comment has been minimized.

Copy link

commented on spyne/util/meta.py in 5f18187 Mar 18, 2014

this file needs

  1. Spyne's license header
  2. Reference to the original author's work
@plq

This comment has been minimized.

Copy link

commented on ac672bb Mar 18, 2014

why did you have to do this?

@plq

This comment has been minimized.

Copy link
Member

commented Mar 18, 2014

ok i've had a cursory look and this looks just fine. do you think we need tests here?

@DXist

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2014

don't we use the metaclass for both python 3 and 2 but in 3 it's just a noop?
yes
why did you have to do this?

I don't like that I have to customize each type to support field ordering:

class MyComplexModel(ComplexModel):
     foo = Integer.customize()
     bar = Unicode.customize()
     ....

do you think we need tests here?
yes. I will prepare some.

Why do we support 'name' ordering? 'random' is for previous behaviour, 'declared' is for deterministic ordering. Does anybody use 'name'?

As I understand default ordering should be 'random' one.

@plq

This comment has been minimized.

Copy link
Member

commented Mar 19, 2014

Hello,

When I said "Why did you have to do this", it was referring to the commit where you did the refactoring for odict to directly inherit from dict

Why do we support 'name' ordering? 'random' is for previous behaviour, 'declared' is for deterministic ordering. Does anybody use 'name'?

no. it's a hack we came up with to have deterministic ordering of some sort.

As I understand default ordering should be 'random' one.

yes.

@plq

This comment has been minimized.

Copy link
Member

commented Mar 19, 2014

do you think we need tests here?

yes. I will prepare some.

I think we should test for the portability of code object inspection.

Also, you can use the run_tests.sh script if you want to run the test on multiple platforms. It's best to run them locally -- I don't recommend you to use jenkins at cloudbees while you're writing them because it starts from blank environment every time, so testing the tests while waiting for CPython to compile every time would be very annoying.

@DXist

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2014

When I said "Why did you have to do this", it was referring to the commit where you did the refactoring for odict to directly inherit from dict

We have to use odict as attributes container to maintain right ordering. Python requires this container to be dict instance:

>>> type('A', (object,), object())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type() argument 3 must be dict, not object

I think we should test for the portability of code object inspection.

Right. As for IronPython it should be built with –X:FullFrames option.

@DXist

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2014

Changes highlights:

  • Django mapper switched to ordereddict, added hint for users of custom ComplexModelMeta metaclasses
  • complex model tests that don't require lxml are isolated
  • test_multi_python setup.py command is added to run them
  • tox config is prepared to run multipython tests on PyPy, Python 2 and Python 3. Jython and IronPython implementations don't work out of the box with tox (at least on my machine).
  • Enabled Prepareble hack for Python 3 to support add_metaclass decorators.

Tox changes PYTHONHASHSEED at each run by default so it's useful for finding issues with nondeterministic ordering.

DXist added some commits Mar 20, 2014

Restore random ordering as early as possible
New attributes shouldn't be set before restoration because we will not
get original value distribution in unordered dict
@DXist

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2014

Have you looked into these commits?

@plq

This comment has been minimized.

Copy link
Member

commented Apr 2, 2014

i'll be back to office friday. i'll have a look first thing in the morning.

@plq

This comment has been minimized.

Copy link
Member

commented Apr 13, 2014

wow, you got a lot done here.

@plq

This comment has been minimized.

Copy link
Member

commented Apr 13, 2014

So, the tests you're running in python 3 are only the ones that pass in python 3 yes?

@plq

This comment has been minimized.

Copy link
Member

commented Apr 13, 2014

Were there any changes to the tests that you moved to multi_python.py or were they moved verbatim?

@plq

This comment has been minimized.

Copy link
Member

commented Apr 13, 2014

Maybe we could add a small paragraph explaining how spyne.util.meta works in the module docstring.

We could even add a nice tl;dr like:

It's HORRIBLE!!!!!

:)

@plq

This comment has been minimized.

Copy link
Member

commented Apr 13, 2014

"""
The preparable class inspects the frame stack in order to locate the class definition, re-parses it to get declaration order from the AST and uses that information to order elements.
"""

@plq

This comment has been minimized.

Copy link
Member

commented Apr 13, 2014

ok this looks fine. merging. thanks a lot for your time and apologies for taking so long.

plq added a commit that referenced this pull request Apr 13, 2014

Merge pull request #343 from DXist/feature/transparent_declared_for_p…
…ython2

Feature/transparent declared for python2

@plq plq merged commit b7a47da into arskom:master Apr 13, 2014

1 check passed

default The Travis CI build passed
Details
@DXist

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2014

So, the tests you're running in python 3 are only the ones that pass in python 3 yes?

Mostly I've isolated multipython tests to run them on PyPy.

It's HORRIBLE!!!!!

Yes. It's probably one of the craziest hacks I've ever done! It's up to you to put as much expressive comments as you like :-)

Were there any changes to the tests that you moved to multi_python.py or were they moved verbatim?

I just separated them. I think I haven't missed anything.

plq added a commit to plq/spyne that referenced this pull request Apr 22, 2014

plq added a commit to plq/spyne that referenced this pull request Apr 22, 2014

@plq

This comment has been minimized.

Copy link
Member

commented Apr 23, 2014

I'm happy to report that I can't break any existing code with declare_order='declared' after the above patches. interesting :)

plq added a commit to plq/spyne that referenced this pull request May 5, 2014

@plq

This comment has been minimized.

Copy link
Member

commented May 5, 2014

to all the users of this patch: this won't work unless you explicitly have declare_order='declared' in Attributes.

@plq

This comment has been minimized.

Copy link
Member

commented May 5, 2014

Also, if you have this patch enabled, your daemon won't even start when compiled with Nuitka.

plq added a commit to plq/spyne that referenced this pull request May 5, 2014

plq added a commit to plq/spyne that referenced this pull request May 5, 2014

@DXist DXist deleted the DXist:feature/transparent_declared_for_python2 branch Jun 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.