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

refactor get_data_for_analysis #246

Conversation

igusher
Copy link

@igusher igusher commented Jan 22, 2019

My main goal is to change responsibilities a bit between classes in order to modularize computations so that later we can reuse some chunks in distributed environment.

The first thing I want to do - is to encapsulate computational logic in StatisticalTest. As it will be a building block in distributed environment and one StatisticalTest can run independently from the other. (Of course later StatisticalTestSuite will collect all Tests and correct them)
....
And I will do it in baby-steps so that you can see if I break any existing assumptions.

@igusher
Copy link
Author

igusher commented Jan 22, 2019

Well, exactly this change is trying to improve code according to Law of Demeter

@igusher igusher force-pushed the refactoring/encapsulate-some-methods-into-statistical-test branch 2 times, most recently from f2b9b40 to 9349f6f Compare January 22, 2019 18:08
@igusher igusher force-pushed the refactoring/encapsulate-some-methods-into-statistical-test branch from 9349f6f to e3ac521 Compare January 22, 2019 18:12
@igusher igusher force-pushed the refactoring/encapsulate-some-methods-into-statistical-test branch from c6d4608 to 5133b27 Compare January 22, 2019 18:14
@coveralls
Copy link

coveralls commented Jan 22, 2019

Coverage Status

Coverage increased (+0.1%) to 92.406% when pulling a567607 on igusher:refactoring/encapsulate-some-methods-into-statistical-test into 99908d1 on zalando:master.

@igusher
Copy link
Author

igusher commented Jan 22, 2019

I've changed classes which are derived from JsonSerializable.
Could it potentially break existing serialization ?

@daryadedik
Copy link
Contributor

@igusher can it be reviewed or it's in progress?

@igusher
Copy link
Author

igusher commented Jan 23, 2019

@daryadedik Yes, this one is ready for review.

@aaron-mcdaid-zalando
Copy link
Collaborator

This breaks very many of the ExpanService tests

The errors are usually something to do with serializing weakref:

AttributeError: 'weakref' object has no attribute '__dict__'
ERROR:ExpanService:'weakref' object has no attribute '__dict__'

@igusher
Copy link
Author

igusher commented Jan 28, 2019

@aaron-mcdaid-zalando What is an ExpanService? I'm not aware of it. I'm just a random contributor to an exapn lib :)
Do you know when and how it attempts to serialize weakrefs? which objects are those?

@igusher igusher closed this Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants