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.

None yet

3 participants