Skip to content
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

Review and discuss the first refactorings of models.py #1

Open
bhaugen opened this issue Feb 28, 2017 · 17 comments
Open

Review and discuss the first refactorings of models.py #1

bhaugen opened this issue Feb 28, 2017 · 17 comments
Labels

Comments

@bhaugen
Copy link
Contributor

bhaugen commented Feb 28, 2017

Looking at https://github.com/django-rea/nrp/compare/models_refactor

Looks like it breaks down somewhat into layers:

  • core: looks like the "Reality" or "What has occurred" layer of REA. See layer diagrams below.
  • types: the "Recipe" or "What could be" layer.
  • schedule: part of the "Plan" or "What is scheduled" layer.
  • processes: overlap between "Plan"/"What is scheduled" and "Reality"/"What has occurred"
  • trade: separating out Exchanges
  • behavior: value equations and distributions plus facets and patterns
  • misc: what it is

NRP layers:
recipeplanreality

Bill McCarthy's REA layers:
mccarthy_layers

@escanda great work! Let me know if I summarized correctly. Will start reviewing and discussing in comments.

@bhaugen
Copy link
Contributor Author

bhaugen commented Feb 28, 2017

I'll start with some questions and analysis.

@escanda what were your goals for this refactoring? What did you want to accomplish? And what did you not want to accomplish?

I can think of several goals for refactoring models:

  • smaller and less intimidating models files.
  • separating into units of work: that is, groups of model classes that somebody might work on together, that might be logically connected in features.
  • separating into modules that one or other group might want to use, or not use (which might be done better by separating into apps than just models files).
  • separating into components which somebody might want to override with their version of the same component.
  • other goals? I am sure I have not thought of everything.

When @fosterlynn and I discussed refactoring in a long car ride, I thought of a table, like this, with rows for the layers and the columns for the R, E, and A, and adding Process and Exchange:

Layer Resource Event Agent Process Exchange
Recipe/Type ResourceType EventType, CommitmentType AgentType ProcessType ExchangeType
Plan Commitment Process Exchange
Reality Resource EconomicEvent Agent Process Exchange

That's a simplified table and omits a lot of the "supporting" classes that would need to go with the majors. And note, Process and Exchange appear on two layers.

But it would be possible to refactor by layer (which is pretty much what @escanda did, if I understood correctly), or by column, or by both (radical refactoring into possibly the smallest practical modules).

And it would be possible to refactor according to other criteria ( @fosterlynn is saying "functionality" off to the side right now...;-)

I am not advocating anything here, just thinking in public. Before advocating, I want to know more about which modules would make sense for the three groups involved in the code base now: Freedom Coop (OCP), GoPacifia (DEEP), and Matrioshka (OCP for now). That would be another reason for factoring out features that some groups don't want to use.

@fosterlynn
Copy link
Contributor

And it would be possible to refactor according to other criteria ( @fosterlynn is saying "functionality" off to the side right now...;-)

... but not as a recommendation, just to say there is probably some sweet spot between data driven and functionality driven refactoring. I don't know what it is in this case. I'd like to spend some focused time on it, hoping to do that today.

Before advocating, I want to know more about which modules would make sense for the three groups involved in the code base now

👍

@fosterlynn
Copy link
Contributor

fosterlynn commented Feb 28, 2017

OK, here is my first attempt to do a combined resource driven and function driven set of groupings on the data side. Could be very wrong, I think a lot of this will come out more clearly when we try to do the views too. But at least hopefully some food for thought.

#agent with type info, could create a separate agent app as well as support all of NRP
from django_rea.valueaccounting.models.agent import (
    EconomicAgent,
    AgentUser, #should this be here? not sure how we are separating out the account stuff
    AgentAssociation,
    AgentType,
    AgentAssociationType,
)

#could support a separate app for designs, as well as just what is needed for most of NRP - 
#but decided to put exchange here too because eventually we want to have combined 
#process and exchange recipes
from django_rea.valueaccounting.models.recipe import (
    EconomicResourceType,
    ResourceClass,
    ResourceTypeSpecialPrice,
    CommitmentType,
    EventType,
    ProcessType,
    ResourceTypeList,
    ResourceTypeListElement,
    Feature,
    Option,
    SelectedOption, #double check this is part of options
    AgentResourceType, #not sure if this appears anywhere on the UI now, and too bad to put it
                # here, because now agent becomes required - maybe we can eliminate it
    Unit,
    ExchangeType,
    TransferType,
)

#could support a resource sharing app, as well as part of the core layer of NRP
from django_rea.valueaccounting.models.resource import (
    EconomicResource,
    ResourceState,
    AgentResourceRole,
    AgentResourceType,
    AgentResourceRoleType,
)

#could combine this and process and exchange - but I think on the view/templace 
#side it is good to separate process and exchange
from django_rea.valueaccounting.models.event import (
    EconomicEvent,
    CachedEventSummary,
    EventSummary,
    AccountingReference,
)

from django_rea.valueaccounting.models.process import (
    Process,
)

from django_rea.valueaccounting.models.trade import (
    Exchange,
    Transfer,
)

from django_rea.valueaccounting.models.schedule import (
    Commitment,
    Reciprocity, #possibly not used?
    Order,
)

#only needed if you are doing contributory accounting
from django_rea.valueaccounting.models.distribution import (
    Distribution,
    DistributionValueEquation,
    ValueEquation,
    ValueEquationBucket,
    ValueEquationBucketRule,
    IncomeEventDistribution,
    Claim,
    ClaimEvent,
)

#both process and exchange facet-value config
from django_rea.valueaccounting.models.facetconfig import (
    Facet,
    FacetValue,
    ResourceTypeFacetValue,
    PatternFacetValue,
    ProcessPattern,
    PatternUseCase,
    TransferTypeFacetValue,
    UseCase, #this may need to go somewhere more general, but I think this is most if not all of its usage
    UseCaseEventType,
)

#needed in both agent and resource
from django_rea.valueaccounting.models.location import (
    Location,
)

from django_rea.valueaccounting.models.misc import (
    HomePageLayout,
    Help,
)

@bhaugen
Copy link
Contributor Author

bhaugen commented Feb 28, 2017

Reciprocity, #possibly not used?

Yeah, it's dead, let's remove it.

@ghost
Copy link

ghost commented Feb 28, 2017

But it would be possible to refactor by layer (which is pretty much what @escanda did, if I understood correctly),

I must confess I took Pavel's book and figured out some of the naming conventions. I must say, I like a lot @fosterlynn 's approach. I am a REA noob so please bear with me :) I must dig deeper into REA modelling.

Before advocating, I want to know more about which modules would make sense for the three groups involved in the code base now: Freedom Coop (OCP), GoPacifia (DEEP), and Matrioshka (OCP for now). That would be another reason for factoring out features that some groups don't want to use.

Imho @fosterlynn initial proposal is really solid.

@bhaugen
Copy link
Contributor Author

bhaugen commented Feb 28, 2017

What would you all think of separating the models into their own app(s) and the views and templates into another app or apps? That way, each group could create their own UI, to the extent that they wanted to do so? We'd need to provide a default UI so new groups could get up and running with a complete-ish system.

The API is already its own app, but might want to be split up too, depending on how we split the models. (I think the API is dependent on the models, but not on the views.)

@fosterlynn reminds me that we might have some logic in the views that should really be in the models or utils, so we might need to do some cleanup.

@fosterlynn
Copy link
Contributor

we might have some logic in the views that should really be in the models or utils, so we might need to do some cleanup.

And by "might" I mean I'm sure we do. There's even a little hidden in the forms. But I think this can all or mostly be later in the refactoring process, first would be to get it split up and running as is, and get it up to the latest Django.

@fosterlynn
Copy link
Contributor

I am a REA noob so please bear with me :) I must dig deeper into REA modelling.

@escanda you are doing amazing! ❤️ Some of this is also NRP specific (beyond REA), sometimes for good reasons, sometimes for just historical / get-it-out-there / lots-of-experimenting kinds of reasons.

I am really pleased you are pushing on this refactoring, it is exciting to think of this getting a good clean up! And more exciting to think that it could be a viable open source project for multiple coordinating groups! 😄

@ghost
Copy link

ghost commented Mar 1, 2017

If everyone is okay with @fosterlynn 's proposal, I can take on the task of accommodating models into this new structure in 4 hours approximately. I will, nonetheless, instead of following the initial radical-black-boxed refactoring approach, post updates here so you can review, and finally create a pull request for final review before merging into master.

@bhaugen
Copy link
Contributor Author

bhaugen commented Mar 1, 2017

See also #2

@fosterlynn
Copy link
Contributor

If everyone is okay with @fosterlynn 's proposal, I can take on the task of accommodating models into this new structure in 4 hours approximately.

If we use ABCs, do we even want to split the models? Or would that be in the next "layer"? (I don't know the answer.)

@bhaugen
Copy link
Contributor Author

bhaugen commented Mar 1, 2017

@escanda

instead of following the initial radical-black-boxed refactoring approach,

Yeah, don't go radical (I assume you mean all the cells in that table above), or at least not yet.

@fosterlynn
Copy link
Contributor

I can take on the task of accommodating models into this new structure in 4 hours approximately.

Other question, should we do some Django upgrades first? (Just asking, I am more or less following but not technical enough to have an opinion. It does seem like we need to make a few technical decisions first though?)

@ghost
Copy link

ghost commented Mar 1, 2017

Sorry, I just came back to my computer, I am starting in a few minutes.

If we use ABCs, do we even want to split the models? Or would that be in the next "layer"? (I don't know the answer.)

We want. And furthermore, we want to follow a similar schema to the models naming convention once we start splitting things into apps.

Yeah, don't go radical (I assume you mean all the cells in that table above), or at least not yet.

I won't :)

Other question, should we do some Django upgrades first? (Just asking, I am more or less following but not technical enough to have an opinion. It does seem like we need to make a few technical decisions first though?)

Totally right :) That's why I proposed my hidden agenda openly in here. For a first iteration, that is, using your naming and file conventions, we don't need yet to upgrade Django. But in the near future, once we start splitting into apps, we do.

ghost pushed a commit that referenced this issue Mar 1, 2017
@numapanuma
Copy link
Contributor

numapanuma commented Mar 3, 2017 via email

@numapanuma
Copy link
Contributor

numapanuma commented Mar 3, 2017 via email

@bhaugen
Copy link
Contributor Author

bhaugen commented Mar 3, 2017

What was the reciprocity function? Wasn't it the "counter transfef in an
exchange event?

No, it was a previous attempt to do the same thing. Never got used. The redesign of exchanges makes it totally obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants