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

wraputils.lua setup_property_access shouldn't override class methods #482

Merged
merged 6 commits into from Jul 25, 2016

Conversation

Projects
None yet
4 participants
@mike1808
Contributor

mike1808 commented Jul 17, 2016

This PR fixes bug when you create multiple instances for exposed objects. For example, when you call splash#call_later you create a new instance of _ExposedTimer Python class and Timer Lua class.

""")
self.assertStatusCode(resp, 200)
self.assertEqual(resp.json(), [False, False, False, False, False, False])

This comment has been minimized.

@mike1808

mike1808 Jul 17, 2016

Contributor

If you run this test w/o this fix you will get [False, True, True, True, True, False]. This happens because when setup_property_access is called it sets __index and __setindex metamethods to the provided class. And that way only the first and the last created instances will work correctly, because for the class (table) of the first instance is created without any overrides and the last instance works because its class methods has right self arguments.

@mike1808

mike1808 Jul 17, 2016

Contributor

If you run this test w/o this fix you will get [False, True, True, True, True, False]. This happens because when setup_property_access is called it sets __index and __setindex metamethods to the provided class. And that way only the first and the last created instances will work correctly, because for the class (table) of the first instance is created without any overrides and the last instance works because its class methods has right self arguments.

This comment has been minimized.

@immerrr

immerrr Jul 18, 2016

Contributor

This comment belongs in the source code, not here.

@immerrr

immerrr Jul 18, 2016

Contributor

This comment belongs in the source code, not here.

Show outdated Hide outdated splash/lua_modules/wraputils.lua Outdated
@mike1808

This comment has been minimized.

Show comment
Hide comment
@mike1808

mike1808 Jul 17, 2016

Contributor

Also, I didn't find a way to test @lua_property methods (getters and setters).

Contributor

mike1808 commented Jul 17, 2016

Also, I didn't find a way to test @lua_property methods (getters and setters).

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 17, 2016

Current coverage is 87.87% (diff: 100%)

Merging #482 into master will not change coverage

@@             master       #482   diff @@
==========================================
  Files            40         40          
  Lines          4875       4875          
  Methods           0          0          
  Messages          0          0          
  Branches        677        677          
==========================================
  Hits           4284       4284          
  Misses          441        441          
  Partials        150        150          

Powered by Codecov. Last update e19bcef...f31adf0

codecov-io commented Jul 17, 2016

Current coverage is 87.87% (diff: 100%)

Merging #482 into master will not change coverage

@@             master       #482   diff @@
==========================================
  Files            40         40          
  Lines          4875       4875          
  Methods           0          0          
  Messages          0          0          
  Branches        677        677          
==========================================
  Hits           4284       4284          
  Misses          441        441          
  Partials        150        150          

Powered by Codecov. Last update e19bcef...f31adf0

Show outdated Hide outdated splash/lua_modules/splash.lua Outdated
Show outdated Hide outdated splash/lua_modules/wraputils.lua Outdated
Show outdated Hide outdated splash/lua_modules/wraputils.lua Outdated

@kmike kmike self-assigned this Jul 21, 2016

@immerrr

This comment has been minimized.

Show comment
Hide comment
@immerrr

immerrr Jul 25, 2016

Contributor

LGTM

Contributor

immerrr commented Jul 25, 2016

LGTM

@immerrr immerrr merged commit fa2f50e into scrapinghub:master Jul 25, 2016

2 checks passed

codecov/patch Coverage not affected when comparing e19bcef...f31adf0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment