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

Turn global_state into interfaces #428

Merged
merged 12 commits into from
Feb 18, 2021

Conversation

AnthraX1
Copy link
Contributor

@AnthraX1 AnthraX1 commented Feb 15, 2021

The main purpose of this humongous PR is to consolidate properties of data instance distributed in global variables into properties of newly created DtaleInstance and DefaultStore class. This allows easy expansion of data instance properties and easy implementation of custom store classes. It would also allow multiple store instances to be created without too much modification. I managed to fix as many tests as possible but there are still a few keep getting error.

dtale/views.py Outdated
Comment on lines 175 to 176
if data_id != None:
data_id = int(data_id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data_id = data_id if data_id is None else int(data_id)

Suggested change
if data_id != None:
data_id = int(data_id)
data_id = data_id if data_id is None else int(data_id)

@aschonfeld aschonfeld self-requested a review February 16, 2021 20:07
@AnthraX1 AnthraX1 requested a review from aschonfeld February 18, 2021 03:09
Copy link
Collaborator

@aschonfeld aschonfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fix that one test and squash your commits, great work!

Oops looks like there are still merge conflicts too.

@aschonfeld aschonfeld merged commit 57493a1 into man-group:master Feb 18, 2021
@AnthraX1 AnthraX1 deleted the better_globalstate branch February 18, 2021 04:18
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.

2 participants