Skip to content
This repository has been archived by the owner on Jun 10, 2021. It is now read-only.

Modularize base.py for readability #90

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Modularize base.py for readability #90

merged 1 commit into from
Dec 18, 2017

Conversation

keyan
Copy link
Contributor

@keyan keyan commented Aug 18, 2017

This is downstream of #89

Partially addresses #64. Though I think things can be much cleaner still, I didn't want to diverge too far without merging first.

The code is mostly the same, but individual classes have been moved into their own classes. Additionally, I created two "helper" modules that include general use functions. In order to avoid a circular dependency I removed the run_interactive method from the Query class and update the associated documentation.

@keyan
Copy link
Contributor Author

keyan commented Dec 18, 2017

@modocache any interest in this change? I'm happy to fix the merge conflicts if you plan on merging.

@modocache
Copy link
Contributor

Yup, sorry! Please rebase and I'll merge, thanks.

@keyan
Copy link
Contributor Author

keyan commented Dec 18, 2017

@modocache no problem, I know this is a really hard one to review so I figured you might not have the bandwidth to accept it.

I rebased and fixed the conflicts with 08331ef and 162d234 manually by applying those changes to the respective modules terminal_helper.py and query.py.

@modocache
Copy link
Contributor

Sweet, thanks! It's great to have this split up, maybe we can start adding unit tests soon. Awesooome!

@modocache modocache merged commit 7ec4d61 into facebookarchive:master Dec 18, 2017
@keyan keyan deleted the kp_modularize branch December 18, 2017 18:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants