philipbelesky
commented
8 years ago Relevant branch is feature/app-restructure. I might try and start on the low hanging fruit such as a Print app
Closed philipbelesky closed 8 years ago
philipbelesky
commented
8 years ago Relevant branch is feature/app-restructure. I might try and start on the low hanging fruit such as a Print app
czlee
commented
8 years ago here was the list I jotted down this afternoon:
It's probably best if we have this thought through before we start rearranging code though. I'm planning on watching all of this video to try and understand how things are meant to be done. Not all of it is relevant but a lot of it is.
czlee
commented
8 years ago On users I think it might be possible to extend the Django user authentication system for our purposes, but I'm not quite sure. Relevant link: https://docs.djangoproject.com/en/1.8/topics/auth/customizing/#custom-permissions
The anchor points to the most promising part but the entire page might be relevant.
czlee
commented
8 years ago First pass of grouping models, to be refined. I just did it at a glance, haven't put much thought into matching with either list of app-divisions above. Also, some attention should probably be paid to ForeignKey dependencies with this, though I'm not sure how much we can do about it.
class Tournament(models.Model)
class Round(models.Model)
class VenueGroup(models.Model)
class Venue(models.Model)
class Division(models.Model)
class Person(models.Model)
class BreakCategory(models.Model)
class BreakingTeam(models.Model)
class Adjudicator(Person)
class AdjudicatorAllocation(object)
class AdjudicatorConflict(models.Model)
class AdjudicatorAdjudicatorConflict(models.Model)
class AdjudicatorInstitutionConflict(models.Model)
class Region(models.Model)
class Institution(models.Model)
class Team(models.Model)
class TeamVenuePreference(models.Model)
class TeamPositionAllocation(models.Model)
class Speaker(Person)
class Debate(models.Model)
class DebateTeam(models.Model)
class DebateAdjudicator(models.Model)
class Checkin(models.Model)
class ActiveVenue(models.Model)
class ActiveTeam(models.Model)
class ActiveAdjudicator(models.Model)
class Submission(models.Model)
class AdjudicatorFeedbackAnswer(models.Model)
class AdjudicatorFeedbackBooleanAnswer(AdjudicatorFeedbackAnswer)
class AdjudicatorFeedbackIntegerAnswer(AdjudicatorFeedbackAnswer)
class AdjudicatorFeedbackFloatAnswer(AdjudicatorFeedbackAnswer)
class AdjudicatorFeedbackStringAnswer(AdjudicatorFeedbackAnswer)
class AdjudicatorFeedbackQuestion(models.Model)
class AdjudicatorFeedback(Submission)
class AdjudicatorTestScoreHistory(models.Model)
class BallotSubmission(Submission)
class SpeakerScoreByAdj(models.Model)
class TeamScore(models.Model)
class SpeakerScore(models.Model)
class Motion(models.Model)
class DebateTeamMotionPreference(models.Model)
class ActionLog(models.Model)
class Config(models.Model)Cool; I have been working on standings and public as neither requires separate models. Although maybe not every logic grouping of models requires an app unless it has a reasonably distinct piece of functionality in the UI?
For example Region and Institution sound like decent candidates for the "Core" app; although Region potentially could be shifted to Allocations as that is its functional use.
czlee
commented
8 years ago Maybe. In my head there was "things that are properties of the tournament" and "things that describe participants of the tournament". How useful a division that is I'd have to think about further.
My instinct is against spinning the public interface into an app of its own though. I sort of think, e.g., the public version of the draw and the private version of the draw are part of the same function.
philipbelesky
commented
8 years ago Yes. And so much of the view and template logic is shared (well with much room for improvement) that the public/private functionality really should be in the same place (from a code clarity/maintainability standpoint)
philipbelesky
commented
8 years ago One slight complication I've noted. In terms of the URL scheme, say for the a Break app. You have a path coming from /break/ (on the public side) and /admin/break/ (on the admin side) so if those are both routed to break/urls.py like so:
# Break App
url(r'^break/$', include('break.urls')),
url(r'^admin/break/', include('break.urls')),But then within break/urls.py the url paths need different names for the admin and public functions, IE:
url(r'^teams/(?P<category>\w+)/$', 'breaking_teams', name='breaking_teams'),
url(r'^public_teams/(?P<category>\w+)/$', 'public_breaking_teams', name='public_breaking_teams'),Chucking "public_" in front of all the non-admin functions has been my solution so far. Which makes things slightly more verbose than needed, although is nice in terms of code clarity. We could also just move the full paths to break/urls.py, ie:
url(r'^admin/adjudicators/break/teams/(?P<category>\w+)/$', 'breaking_teams', name='breaking_teams'),But that does seem to run contrary to the purpose of modularisation. Anyway, not a big problem, and easily refactored later down the line if needed. If no other solution prevents itself, I'm ok with the "public_" prefix option also.
czlee
commented
8 years ago Can we split into two urls files per app, urls_public.py and urls_admin.py?
The public_ prefix probably isn't ideal, like intuition-wise for public users, but it's not too unsettling.
philipbelesky
commented
8 years ago Ah right, that makes much more sense. Also in weird small things: I've opted for 'breaking' and 'printing' as app names to avoid keyword clashes. Probably wouldn't ever happen, but best to be safe?
Similarly, with the small one-off scripts like breaking.py and standings.py. should they be renamed now that they are in apps of the same name? So instead of import breaking.breaking.get_breaking_teams it would be breaking.methods / breaking.utils or something?
Also I had a go at spinning off the first app with a model: the breaking app. Current head of the branch has BreakingTeam and BreakCategory.breaking_teams commented out because they break the initial migration. Something to do with ManytoMany / through models across apps that I haven't been able to work out yet. I probably won't come back to it for another day or so if you want to have a look at it.
czlee
commented
8 years ago (1) That's not just being safe, you don't have a choice—you can't clash with keywords! print is no longer a keyword in Python 3, but we don't have any Python 3 migration plans and it's probably still good practice to avoid it anyway. I'd use printable instead of printing though :stuck_out_tongue:
It's not entirely clear to me that printable should be its own application, as opposed to part of a results application and an adjudicator feedback application respectively: the printable versions are tightly coupled to the original forms, and they are sort of part of the same function. That said I also think there's a case for saying it's a different function, so could be swayed—just leaning towards the former, because printable forms don't fit the orthogonality criterion (in reference below).
(2) Maybe. I don't really know, actually. There's nothing wrong with breaking.breaking, even though it looks a little weird at first, I don't think it's actually frowned upon. That said, I sort of agree a more descriptive name is in order. utils is normally used for small peripheral functions, so doesn't seem appropriate. methods might work, but I think it's non-standard. Another possibility is to describe what they are, so e.g. draw.py would become draw/generator.py (draw.generator). Standings will eventually be a "generator" as well. As for breaking... I don't have a good answer there, since the methods are also about retrieval... will think on it... some might argue that, since breaking also writes to the database, even if logic is in its own file it should be accessed via the Manager of something.
There's probably a case for the relevant functions to be put in the __all__ of the app's __init__.py as well.
(3) Noted.
Here is the most authoritative guideline on splitting apps I've found so far. Quite old, actually, but still useful. http://img105.job1001.com/upload/adminnew/2015-04-08/1428444902-ZN8BZ8W.pdf. Pages 227–228 of the book, which is pages 245–246 of the file. Chapter 12, section headings "Drawing the Lines Between Applications" and "Splitting Up the Code-Sharing Application".
Slides to the video I mentioned earlier are at http://media.b-list.org/presentations/2008/djangocon/reusable_apps.pdf.
philipbelesky
commented
8 years ago Other misc thoughts: should Ballot checkins be rolled into a Results app and People checkins be rolled into an Availability app? Or perhaps both be rolled into Availability?
czlee
commented
8 years ago It depends. If ballot checkins fits well into the availability paradigm, i.e. basically uses the same code base, then it should fit in with availability. If not, it should be in results.
Availability is one of those apps that is actually reusable outside debate world. I'd be inclined to write it as if it were (properly) reusable, to keep disciplined if for no other reason.
czlee
commented
8 years ago (Ballot checkins could fit into the availability paradigm, c.f. participant barcode checkins.)
czlee
commented
8 years ago Actually, if it doesn't fit with availability, ballot check-ins should probably be its own thing. It's not part of the core purpose of the results app.
czlee
commented
8 years ago Okay, after reviewing the whole thread and work in progress on the app-restructure branch, here's my current thoughts:
| App | Function |
|---|---|
| core | Manage properties of the tournament. There's room to argue that some of these should be apps in themselves, especially venues. |
| participants | View participants. Also, add/remove/edit participants, but we don't support this (yet). If we ever do a registration system, it should primarily interface with this app. |
| adj_allocation | Allocate adjudicators. |
| draw | Generate and view the draw. |
| availability | Manage the (physical) presence of people or things. |
| adj_feedback | Submit and view adjudicator feedback. |
| results | Submit and view results. |
| motions | Add/remove/edit/release motions. |
| breaking | Generate and view the break. |
| standings | Generate and view standings. |
| action_log | Log all actions. This sort of fingers into all other apps, so the API to other apps should probably be reconsidered. |
| config | Manage tournament configuration. |
| printable | Generate printed forms, but I'm not satisfied this should be its own app, it's tightly coupled to other apps. |
| user | Manage user accounts, but I think for now we hold off and extend the Django authentication framework as necessary. |
# Tournament
class Tournament(models.Model):
class Round(models.Model):
class Division(models.Model):
class VenueGroup(models.Model): # arguably should be in a separate 'venues' app
class Venue(models.Model): # arguably should be in a separate 'venues' app
class Submission(models.Model): # not ideal location -- here because it is generic
# Participants
class Region(models.Model):
class Institution(models.Model):
class Team(models.Model):
class Person(models.Model):
class Speaker(Person):
class Adjudicator(Person): # note, not in allocations or feedback
# Adjudicator allocation
# In principle, references to Adjudicator should be generic-ish
# (i.e. the allocation problem should not depend strictly on the particular model)
class DebateAdjudicator(models.Model): # note, moved from 'draw'
class AdjudicatorConflict(models.Model): # should be renamed AdjudicatorTeamConflict
class AdjudicatorAdjudicatorConflict(models.Model):
class AdjudicatorInstitutionConflict(models.Model):
class AdjudicatorAllocation(object): # this is not a model and should be put in a different file
# Draw
# In principle, only debate-specific models should exist here (as opposed to
# things that "exist forever"), though TeamPositionAllocation and
# TeamVenuePreference both break that rule, being things that exist only for the
# purpose of draw generation.
class Debate(models.Model):
class DebateTeam(models.Model):
class TeamPositionAllocation(models.Model): # moved from 'participants', arguable
class TeamVenuePreference(models.Model): # moved from 'participants', arguable
# Availability
class ActiveVenue(models.Model): # these three should be wrapped up into
class ActiveTeam(models.Model): # a single model with ContentType
class ActiveAdjudicator(models.Model):
class Checkin(models.Model):
# if ballot check-ins moved here, Debate.ballot_in should be removed
# Checkin (participant barcode scans) should maybe be combined with ballot
# check-ins using ContentType
# Feedback
# In principle, references to Adjudicator should be generic-ish
# (i.e. the feedback system should not depend strictly on the particular model)
class AdjudicatorFeedbackAnswer(models.Model):
class AdjudicatorFeedbackBooleanAnswer(AdjudicatorFeedbackAnswer):
class AdjudicatorFeedbackIntegerAnswer(AdjudicatorFeedbackAnswer):
class AdjudicatorFeedbackFloatAnswer(AdjudicatorFeedbackAnswer):
class AdjudicatorFeedbackStringAnswer(AdjudicatorFeedbackAnswer):
class AdjudicatorFeedbackQuestion(models.Model):
class AdjudicatorFeedback(Submission)
class AdjudicatorTestScoreHistory(models.Model): # arguably should be spun to a different app
# and made reusable - the concept of a history
# is not unique to test scores
# Results
class BallotSubmission(Submission):
class SpeakerScoreByAdj(models.Model):
class SpeakerScore(models.Model):
class TeamScore(models.Model):
# Motions
class Motion(models.Model):
class DebateTeamMotionPreference(models.Model): # note, not in draw
# Breaking
class BreakCategory(models.Model):
class BreakingTeam(models.Model):
# Action log
class ActionLog(models.Model):
# Config
class Config(models.Model):Also, I think general templates should stay in the top-level templates folder, and app-specific templates in templates folders within each app. Thoughts?
philipbelesky
commented
8 years ago Sounds good. The work I've been doing is mostly to test out how the process of app-splitting works, so re-merging things like printing or renaming the apps is no problem. Existing work should definitely not be considered final.
Question: where would the import functionality live — under Participants? I'm referring to both the existing management command and any future GUIs. It seems like anything data-editing or data-importing related is always going to break the orthogonality requirement.
czlee
commented
8 years ago Ummm... I just forgot about it in that table. :flushed: I meant to include it as its own app and missed it.
Yeah, it's a bit odd, since you're right, it does break orthogonality. At first glance I'm inclined to say it's still a separate workflow and it's coupled to like everything so there's not much we can do about it. There's an argument that each module should take care of its own importing requirements, and some top-level app should implement the command that strings them all together, but I'm not sure that that would be very practical.
That said, since importer is a single process coupled to everything and each printable form is directly coupled to a particular app (results and adj_feedback respectively), my instinct is that the two are (or at least could be) sufficiently non-analogous to get different treatment. I suppose you could argue that you want to print off everything at once, but I'm not sure how true that is. And the randomized URLs through another spanner in there too—I think if we're doing it properly, the hash should not be part of the Adjudicator and Team models, they should be in their own database table with ContentType again. I'm not sure where the balance (between decoupling models with ContentType and coupling models) to strike is, I guess the way to find out is to try...
czlee
commented
8 years ago Related fields, with current app divisions but exceptions noted below. It's not intrinsically bad to have lots of interlinking (it's inevitable to a degree) but where there is avoidable excessive cross-linking it may give some indication of where another arrangement might work better.
Changes to previous app divisions:
# ==============================================================================
# Tournament
class Tournament(Model):
ForeignKey(Round) # current_round
class Round(Model):
ForeignKey(Tournament)
ForeignKey(BreakCategory) # links to breaking
ManyToManyField(Person, through=Checkin) # links to participants, availability
ManyToManyField(Venue, through=ActiveVenue) # links to venues, availability
ManyToManyField(Adjudicator, through=ActiveAdjudicator) # links to participants, availability
ManyToManyField(Team, through=ActiveTeam) # links to participants, availability
class Division(Model):
ForeignKey(Tournament)
ForeignKey(VenueGroup) # links to venues
# ==============================================================================
# Venues
class VenueGroup(Model):
pass
class Venue(Model):
ForeignKey(VenueGroup)
ForeignKey(Tournament) # links to tournament
# ==============================================================================
# Participants
class Region(Model):
ForeignKey(Tournament) # links to tournament
class Institution(Model):
ForeignKey(Region)
class Team(Model):
ForeignKey(Institution)
ForeignKey(Tournament) # links to tournament
ForeignKey(Division)
ManyToManyField(BreakCategory) # links to breaking
ManyToManyField(VenueGroup, through=TeamVenuePreference) # links to venues, draw
class Person(Model):
pass
class Speaker(Person):
ForeignKey(Team)
class Adjudicator(Person):
ForeignKey(Institution)
ForeignKey(Tournament) # links to tournament
ManyToManyField(Institution, through=AdjudicatorInstitutionConflict) # links to allocations
ManyToManyField(Team, through=AdjudicatorConflict) # links to allocations
# ==============================================================================
# Adjudicator allocation
class DebateAdjudicator(Model):
ForeignKey(Debate) # links to draw
ForeignKey(Adjudicator) # links to participants
class AdjudicatorConflict(Model):
ForeignKey(Adjudicator) # links to participants
ForeignKey(Team) # links to participants
class AdjudicatorAdjudicatorConflict(Model):
ForeignKey(Adjudicator) # links to participants
ForeignKey(Adjudicator) # conflict_adjudicator # links to participants
class AdjudicatorInstitutionConflict(Model):
ForeignKey(Adjudicator) # links to participants
ForeignKey(Institution) # links to participants
# Breaking
class BreakCategory(Model):
ForeignKey(Tournament) # links to tournament
ManyToManyField(Team, through=BreakingTeam) # links to participants
class BreakingTeam(Model):
ForeignKey(BreakCategory)
ForeignKey(Team) # links to participants
# ==============================================================================
# Availability
class Checkin(Model):
ForeignKey(Person) # links to participants
ForeignKey(Round) # links to tournament
class ActiveVenue(Model):
ForeignKey(Venue) # links to venues
ForeignKey(Round) # links to tournament
class ActiveTeam(Model):
ForeignKey(Team) # links to participants
ForeignKey(Round) # links to tournament
class ActiveAdjudicator(Model):
ForeignKey(Adjudicator) # links to participants
ForeignKey(Round) # links to tournament
# ==============================================================================
# Draw
class Debate(Model):
ForeignKey(Round) # links to tournament
ForeignKey(Venue) # links to venues
ForeignKey(Division) # links to tournament
class DebateTeam(Model):
ForeignKey(Debate) # links to draw
ForeignKey(Team) # links to participants
class TeamPositionAllocation(Model):
ForeignKey(Round) # links to tournament
ForeignKey(Team) # links to participants
class TeamVenuePreference(Model):
ForeignKey(Team) # links to participants
ForeignKey(VenueGroup) # links to venues
# ==============================================================================
# Submissions (abstract)
class Submission(Model):
ForeignKey(settings.AUTH_USER_MODEL) # submitter
ForeignKey(settings.AUTH_USER_MODEL) # confirmer
# ==============================================================================
# Adjudicator feedback
class AdjudicatorFeedbackAnswer(Model):
ForeignKey(AdjudicatorFeedbackQuestion)
ForeignKey(AdjudicatorFeedback)
class AdjudicatorFeedbackBooleanAnswer(AdjudicatorFeedbackAnswer):
pass
class AdjudicatorFeedbackIntegerAnswer(AdjudicatorFeedbackAnswer):
pass
class AdjudicatorFeedbackFloatAnswer(AdjudicatorFeedbackAnswer):
pass
class AdjudicatorFeedbackStringAnswer(AdjudicatorFeedbackAnswer):
pass
class AdjudicatorFeedbackQuestion(Model):
ForeignKey(Tournament) # links to tournament
class AdjudicatorFeedback(Submission):
ForeignKey(Adjudicator) # links to participants
ForeignKey(DebateAdjudicator) # source_adjudicator # links to allocations
ForeignKey(DebateTeam) # source_team # links to draw
class AdjudicatorTestScoreHistory(Model):
ForeignKey(Adjudicator) # links to participants
ForeignKey(Round) # links to tournament
# ==============================================================================
# Results
class BallotSubmission(Submission):
ForeignKey(Debate) # links to draw
ForeignKey(Motion) # links to motions
ForeignKey(BallotSubmission) # copied_from
ForeignKey(DebateTeam) # forfeit # links to draw
class SpeakerScoreByAdj(Model):
ForeignKey(BallotSubmission)
ForeignKey(DebateAdjudicator) # links to allocations
ForeignKey(DebateTeam) # links to draw
class TeamScore(Model):
ForeignKey(BallotSubmission)
ForeignKey(DebateTeam) # links to draw
class SpeakerScore(Model):
ForeignKey(BallotSubmission)
ForeignKey(DebateTeam) # links to draw
ForeignKey(Speaker) # links to participants
# ==============================================================================
# Motions
class Motion(Model):
ForeignKey(Round) # links to tournament
ManyToManyField(Division) # links to tournament
class DebateTeamMotionPreference(Model):
ForeignKey(DebateTeam) # links to draw
ForeignKey(Motion) # links to motions
ForeignKey(BallotSubmission) # links to results
# ==============================================================================
# Action log
class ActionLog(Model):
ForeignKey(settings.AUTH_USER_MODEL)
ForeignKey(Tournament) # links to tournament
ForeignKey(Debate) # links to draw
ForeignKey(BallotSubmission) # links to results
ForeignKey(AdjudicatorTestScoreHistory) # links to feedback
ForeignKey(AdjudicatorFeedback) # links to feedback
ForeignKey(Round) # links to tournament
ForeignKey(Motion) # links to motions
ForeignKey(BreakCategory) # links to breaking
# ==============================================================================
# Config
class Config(Model):
ForeignKey(Tournament) # links to tournamentAnother thing to be considered: where to put core view/utility functions such as admin_required or r2r? A utilities.py in the root directory? The same, but in the core app?
czlee
commented
8 years ago It's normally called util.py or utils.py, but yeah. And yeah... If we have two levels of apps (i.e. a tabbycat in place of the current debate, with all apps discussed so far as sub-apps of tabbycat) then it can be tabbycat/utils.py, alongside tabbycat/tournament etc. If this is permitted and not discouraged practice I'm starting to think it would be more sensible.
On another note, I try not to call things core unless they really are a core of a system, since it's not very descriptive; it seems to me that what we've been describing until now as core is better described as tournament and participants (if we separate those from each other).
czlee
commented
8 years ago Also, I've been staring at the last summary of classes and related fields on-and-off for the last day. On one hand I think some things can, and should, be redesigned: the availability system springs to mind. On the other hand, I'm growing reluctant as a principle to attempt to cleanly separate the apps: in a lot of the related fields, the fact that they are tied to Tabbycat-specific models is, counter-intuitively (or at least contrary to general software wisdom), what makes Tabbycat so powerful. I've been finding it hard to find useful advice on Google/stackoverflow etc. for this kind of decision though. Common wisdom, for example, seems to be that we should always use ManyToManyFields rather than stand-alone intermediary models and there are no good arguments for the latter, but no-one seems to have discussed it much when it crosses app boundaries and breaks modularity. Surely we can't be the first people doing a Django project on a highly-specialized but complex application...
philipbelesky
commented
8 years ago Yea, tournament instead of core makes sense. Unclear about whether tournament and participants need to be separated — they were the lowest priority apps to spin off.
Maybe because I haven't started on the really tightly-bound parts of the app yet, but the parts I've spun off so far have been surpassingly easy to modularise (admittedly my testing has been relatively light, and also because a lot of the cross-functional ties are through things like class methods that happily work between apps). And even if there are still tight ties between the apps, just having things being modularised into apps is a big win in terms of code clarity and maintenance.
My current plan at least is to get the major apps spun off and functional before doing major changes on things like intermediary models / ManytoMany's. I'd rather have this round of architectural changes complete before commencing a second (admittedly related) type of refactor.
czlee
commented
8 years ago I agree. Certainly getting them split up will be a huge first step before any deeper architectural changes start happening.
The tournament and participants separation is arguable—you can argue that managing the tournament itself, and managing the people who are in it, are separate activities, but the practical consequences are less clear. I'm happy to leave this at the bottom of the priority list, or spin participants off when it becomes obvious that it needs to be.
philipbelesky
commented
8 years ago Once the apps have been split up, does it make sense to continue to using prefixes such as "m." for model references? I've been replacing them with new ones (such as "fm." for feedback-app models) but now that the separations/dependancies are more clear, would it make sense just to always make all such imports explicit? IE from feedback.models import AdjudicatorFeedback.
Note I've also been trying to move all import statements to the top of the file (as is generally considered python).
czlee
commented
8 years ago Without having looked at the code, my general rule is to import the module if you're using a lot of things from it, with convenience abbreviation if you're using those things often; and to import using from if the module has a lot of things of which you're only using one or two (or maybe three). If it's a small module that only has a handful of objects, I tend to use from if I'm making references to them often, or import if I'm only making, say, one or two references per object. Basically it's just whichever is less unwieldy… I'm not really sure how the clarity of separations/dependencies would matter, except that I suppose now it's more likely that a particular module will have fewer things imported from it.
Re having them at top of file: Most of the instances I've encountered where this isn't the case are places where circular import dependencies would prevent it. However, I find this can often be resolved by using related managers instead. For example:
def _load_team(self, dt):
# if you try and put this line at the top of the file, circular imports break things
from debate.models import SpeakerScore, TeamScore, DebateTeamMotionPreference
for sss in SpeakerScore.objects.filter(
debate_team = dt,
ballot_submission = self.ballotsub,
):
self.speakers[side][sss.position] = sss.speaker
ts = TeamScore.objects.get(
ballot_submission = self.ballotsub,
debate_team = dt)
self.points[side] = ts.points
self.total_score[side] = ts.score
self.wins[side] = ts.win
self.margins[side] = ts.margin
dtmp = DebateTeamMotionPreference.objects.get(
ballot_submission = self.ballotsub,
debate_team = dt,
preference = 3
)
self.motion_veto[side] = dtmp.motioncan be recast without the import statement as
def _load_team(self, dt):
for ss in self.ballotsub.speakerscore_set.filter(debate_team=dt):
self.speakers[dt][ss.position] = ss.speaker
ts = self.ballotsub.teamscore_set.get(debate_team=dt)
self.points[dt] = ts.points
self.total_score[dt] = ts.score
self.wins[dt] = ts.win
self.margins[dt] = ts.margin
dtmp = self.ballotsub.debateteammotionpreference_set.get(
debate_team=dt, preference=3)
self.motion_veto[dt] = dtmp.motionas I did in c93c28f2ca2c203074ced228cc34e884075f6a28.
As a stylistic habit, I've been trying to use the built-in related managers something.mymodel_set.get(other_field=something_else) rather than MyModel.objects.get(relatedfield=something, other_field=something_else). This includes converting existing instances of the latter, where applicable.
philipbelesky
commented
8 years ago Ok, current HEAD of the branch has removed debate and implemented all of the apps proposed above. To keep them consistent, and to better differentiate between apps and models/files with similar name, they all use pluralised names.
Misc notes:
dj migrate auth before the general dj migrate(this may actually also be true of master/develop)TODO:import and in template namesAre there any other major refactors that should be considered so that the disruption of (eventually) merging this into develop can minimised?
czlee
commented
8 years ago I don't think I've gotten far enough on the config overhaul for it to be affected. If it's not inconvenient to do so, we should probably hold other development until we finish this. I'll get a chance to look at it in the weekend I think.
I'm not sure why we need to differentiate between apps and models/files with similar names, it's pretty common just to have them use the same name where it's sensible to do so. I'm also not sure why grammatical number needs to be consistent among them. Apps, models and files should all use whichever grammatical number actually describes them. For apps, I'd singular-ize availability, action_log, allocation, draw, and feedback. tournament is debatable, since it also covers properties of tournaments; I could go either way on that one.
On another note, do you have any views on migrating to Python 3? If we both think we should do it, we may as well go through all the pain at once. Apparently it's starting to become common now. I don't feel that strongly about it though. Also, haven't checked if all our dependencies support Python 3 yet (the major ones seem to).
czlee
commented
8 years ago I should clarify: by "finish" I do not mean to include some of the other related refactoring I've suggested above, e.g. rewriting availability to be more generic. I just mean the first sub-task of getting the new file structure, well, not to break, I guess.
philipbelesky
commented
8 years ago I'm not sure its intuitive which apps are best as singular or plural; motions and draws both describe a collections of objects that are often created/edited. I found when splitting everything up that its very hard to identify things such as places where a Feedback model might be used, because the word feedback is used so commonly in properties, views, variables, raw SQL commands etc. Having the app name be relatively unique really helps in keeping things less ambiguous, both in refactoring and in reading the code.
As for python3, i'd probably prefer that be started on a different branch. Would the aim be for dual compatibility? Because I think it might be too aggressive to make it backwards incompatible for now. Maybe start a new issue to mark off requirement compatibilities? I note the debug toolbar seems to be incompatible at present.
czlee
commented
8 years ago I think it's obvious to a programmer from context whether something is a module, app or model. Django prefixes all table names with app names, so in SELECT * FROM motions_motion, motions is an app and motion is a model name. The draw in import draw is obviously an app (except in the draw directory itself, if there exists a draw.py, but there's a case independent of this issue that it should be renamed anyway). The allocation in allocation.models.DebateAdjudicator is obviously an app too. The Venue in Venue.objects.get() is a model, as is the motion in debate.motion_set is obviously a model.
Did you find otherwise in code readability, or with find-and-replace? I know it makes find-and-replace more tedious (I faced this with my ballotset/ballotsub find-replace) because you need contextual surroundings to figure out what's what, and I know this splitting is a lot of work, but large-scale find-replaces don't happen too often.
Plural conventions also don't help all that much: oftentimes variables names have to be plural in order to make any sense, as does text in templates. If a uniqueness convention is really necessary, relying on a grammatical construct is a bad one to pick: we're better off prefixing everything with app_ (which I still hold is unnecessary, and also defies all standard conventions).
motions should be plural, but the primary purpose of the draw module is to "generate the draw". ("Generate draws" might be syntactically consistent but I've never heard anyone say that.)
For Python 3, I think if we decide to move to Python 3, we're deciding to leave behind Python 2. If we're not comfortable with dropping support for Python 2, we shouldn't migrate. Large projects can afford to support both during the transition; realistically I don't think we can. As I understand there are syntax differences between the two, so files that work on one may not work on the other. Debug toolbar looks fine with 3: http://py3readiness.org/. But yeah, I guess we should do some research on the others.
philipbelesky
commented
8 years ago Sure, I think the semantics of names are always able to be figured out from context, but having naming conventions that encourage uniqueness make this process easier, and also make it easier to do project-wide searches for particular strings (which happen even outside of major refactors). Unlike an app prefix it maintains the semantics without adding cruft. I don't really get the case for a mix of singular/plural being more semantically correct, at least not when when the purpose of each app is relatively nebulous and well known, but as I mentioned before I don't really mind if it changes.
Also as a mental reminder: we should definitely do a hotfix release from develop to master before integrating this. There are a bunch of fixes in develop, not currently in master, and I would be nervous running a tournament such as Joynt without on this new branch.
czlee
commented
8 years ago Ah okay so are we doing parallel development for a short while? Or have you got all the fixes in?
philipbelesky
commented
8 years ago Hmm, actually if we rolled the current fixes into master as a +0.0.1, it shouldn't then be a problem to merge this into develop. We can always branch off of master for any future hotfixes (which is what gitflow does anyway). I'll go ahead and make the release.
philipbelesky
commented
8 years ago Other misc thoughts/todos, in a form that can be checked-off
data directory or should that be folded into participants or utils?utils/tests should probably be in the app-specific test.py'sUser are commented out.czlee
commented
8 years ago data should be kept where it is, separate from all apps.methods.py; I couldn't find a standard name for this sort of thing. (It's not utils.py; that's normally for internal convenience functions.) But calling it draw/draw.py, breaking/breaking.py is also fine with me. One factor is that a lot of apps will call each other's Python method, e.g. breaking and draw both need to call methods from standings. But if we want we can use import statements in e.g. standings/__init__.py to make it easier (so it's just from standings import get_standings rather than from standings.methods import get_standings).public directory probably doesn't need public in its file name (like public/public_participants.html). Don't know whether to put in public, assistant and admin directories or not. We used to treat admin as a sort of "default" or "normal" form, and this seems to be changing so that the assistant and public are no longer an afterthought; maybe our directories/names conventions should reflect that?philipbelesky
commented
8 years ago czlee
commented
8 years ago Do you think admin template pages should be prefixed admin_ ?
czlee
commented
8 years ago On auth issue, started new discussion at #224.
I've renamed apps as follows:
| Old name | New name |
|---|---|
| action_logs | actionlog |
| allocations | adjallocation |
| availabilities | availability |
| breaks | breakqual |
| draws | draw |
| feedbacks | adjfeedback |
I didn't really intend it to be this way, but for most of them it has the effect of uniqueness as discussed above :wink:
Justifications:
No underscores because PEP 8, emphasis added:
Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.
allocations and feedbacks are ambiguous to someone without prior familiarity with Tabbycat, th. The ambiguity also could potentially constrain us later on, if we write in allocation and feedback functionality for things other than judges.break for a while, and realized that it only deals with qualification for the break, hence the name. There's an argument that the module breaking.py should be renamed similarly. While in theory, it could generate the draw for break rounds, I suspect it won't. I consider break qualification to be a complicated enough exercise that either break round draws should be part of draw, or (if appropriate, but unlikely because there is so much shared functionality with draw) should be in an app of its own.Let me know if any of this sounds questionable to you.
I'm adding AppConfig classes so that the verbose names aren't awkward in the Django admin.
On another note:
importer app. Do we want one? I wasn't sure if we concluded on this.czlee
commented
8 years ago Have fixed team standings and adj allocation, everything is on the app-restructure branch.
czlee
commented
8 years ago Also, I just realized that the use of underscores was discouraged only in Python packages, so technically that recommendation doesn't apply to us. But I dislike underscores in app names anyway. I won't object to their restoration, if desired.
philipbelesky
commented
8 years ago philipbelesky
commented
8 years ago Slight update here:
Basically let me know if this can be marked as completed or not :p
As discussed previously, it would be general best practice to split up our rather monolithic
debateapp into smaller more focused modules.Current thoughts on module structure
SpeakerScoreandBallotSubmissionetc)adjudicatorfolder, probably also things such asDebate,DebateAdjudicator```...?)Ideally, front end resources (mainly HTML and JS) would be bundled into each app, rather than living in a universal folder).