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

Extract AppBase base class #5772

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
3 participants
@traviscrawford
Copy link
Member

traviscrawford commented Apr 30, 2018

Currently support for creating a "bundle" - a self-contained archive that
contains both a binary and loose files - is implemented in JvmApp and
associated helpers. Here we extract an AppBase base class for use by other
languages (e.g.: PythonApp). No functional change in this PR so simplify
reviewing. Adding PythonApp support will happen in a followup PR.

Extract AppBase base class
Currently support for creating a "bundle" - a self-contained archive that
contains both a binary and loose files - is implemented in `JvmApp` and
associated helpers. Here we extract an `AppBase` base class for use by other
languages (e.g.: `PythonApp`). No functional change in this PR so simplify
reviewing. Adding PythonApp support will happen in a followup PR.
@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Apr 30, 2018

To simplify reviewing, since #5704 was getting quite large I extracted some of the refactoring out into a separate PR that makes no functional change.

@@ -153,7 +154,7 @@ def _instantiate_target(self, target_adaptor):

# Instantiate.
if target_cls is JvmApp:

This comment has been minimized.

@jsirois

jsirois Apr 30, 2018

Member

I think this can already be just AppBase.

This comment has been minimized.

@jsirois

jsirois Apr 30, 2018

Member

Since this is green, I'll merge and if this comment was correct, you can hit it in the child PR.

This comment has been minimized.

@traviscrawford

traviscrawford Apr 30, 2018

Member

Agreed - will make that change in the followup.

@benjyw

benjyw approved these changes Apr 30, 2018

@jsirois jsirois merged commit 5a02794 into pantsbuild:master Apr 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@traviscrawford traviscrawford deleted the traviscrawford:travis/refactor-jvm-app branch Apr 30, 2018

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Apr 30, 2018

🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment