Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Factor out a package.team mixin #4422

Merged

Conversation

chadwhitacre
Copy link
Contributor

@chadwhitacre chadwhitacre commented Apr 26, 2017

Part of #4305, follows on #4404.

Minor refactor. Ready for review.

@chadwhitacre chadwhitacre mentioned this pull request Apr 26, 2017
15 tasks
@chadwhitacre chadwhitacre changed the title Factor out a package.team mixin and smooth it out Factor out a package.team mixin Apr 26, 2017
Copy link
Contributor

@dowski dowski left a comment

Choose a reason for hiding this comment

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

What's the advantage of this Team mixin?

"""
return cursor.one( 'SELECT t.*::teams FROM teams t WHERE t.id='
'(SELECT team_id FROM teams_to_packages tp WHERE tp.package_id=%s)'
, (self.id,)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be my recent lack of exposure to Python mixin classes, but this actually feels a bit backward. You mix it in with Package, but instead of Package leveraging some of the functionality of the mixed-in Team, Team leverages things like self.id that must be in Package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's consumers that leverage the synergies contained herein, not necessarily the main Package object itself. The goal here is to keep files reasonably sized (well under 1,000 lines ideally), and the architecture is to have a main Package class (e.g.) that is essentially a container, with API provided by these mixins. But the API itself is called primarily by other objects.

Does that make any sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that makes sense. It seems like the API of Package is a little more opaque that way. It more hides the complexity due to lines of code (if any) than reduces it. But it's no reason to hold up this PR - just my thoughts on the matter now. ;-)

@chadwhitacre chadwhitacre mentioned this pull request Apr 27, 2017
4 tasks
@dowski dowski changed the base branch from project/claim-packages-edit to project/claim-packages April 28, 2017 01:49
@dowski dowski merged commit 826b3ac into project/claim-packages Apr 28, 2017
@chadwhitacre chadwhitacre deleted the project/claim-packages-minor-refactor branch April 28, 2017 08:31
chadwhitacre added a commit that referenced this pull request Apr 28, 2017
chadwhitacre added a commit that referenced this pull request Apr 28, 2017
chadwhitacre added a commit that referenced this pull request May 5, 2017
chadwhitacre added a commit that referenced this pull request May 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants