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

feature: an ActiveLabel module for sharing ActiveNode logic #1453

Open
jorroll opened this issue Dec 15, 2017 · 28 comments
Open

feature: an ActiveLabel module for sharing ActiveNode logic #1453

jorroll opened this issue Dec 15, 2017 · 28 comments

Comments

@jorroll
Copy link
Contributor

jorroll commented Dec 15, 2017

I think issue #1424 has started to bloom into multiple issues. I'm opening this up to continue the discussion of creating some sort of includeable ActiveLabel module functionality.

Preface

@cheerfulstoic began this conversation in #1424 but I didn't really "get it" (see the use case) until, unrelated, I just embarked on a side quest to benchmark the effect of label ordering on query time. As part of this, I downloaded the Neo4j example movie database to test against. Lo-and-behold, as I tried to use ActiveNode to model the data, I begun to see problems....

I initially thought to model the database like so:

class Person 
  include Neo4j::ActiveNode

  property :name

  class User < Person
    self.mapped_label_name = "User"

    property :login    
    property :password    
    property :roles    
  end

  class Actor < Person
    include InShowbusiness

    self.mapped_label_name = "Actor"
  end

  class Director < Person
    include InShowbusiness  

    self.mapped_label_name = "Director"
  end
end

module InShowbusiness
  extend ActiveSupport::Concern

  included do
    property :biography    
    property :lastModified    
    property :version    
  end
end

If you check out the example movie database, you'll notice problems with the above though. This is because a node can have :Person:Actor:User labels associated with it (i.e. can be both an Actor and a User at the same time). So inheritance doesn't work, because then a node would be wrapped as either an Actor or a User, but would not be both (when it should be both).

Adding multiple labels to Person (such as using modules) doesn't work either though, as then Person will always have those labels (which is inappropriate). So :User, :Actor, and :Director labels are truly optional to the person node, except their presence alter's the properties of the person node. In the case of :User, it adds properties.

The exciting part...

So after reading the ideas in #1424 and trying to solve the movie database problem, here's my take which, the more I think about, the more I like. This is definitely similar to what @leehericks wrote down in #1424 -- I just didn't understand it at the time. (it's also possible that this is exactly what @cheerfulstoic and @leehericks where getting at over in #1424)

class Person
  include Neo4j::ActiveNode
  include User
  include Actor
  include Director

  property :name

  # If the person node has the label associated with User do ...

  if_is(User) do
    def admin_powers(id)
      Person.find(id).destroy
    end
  end

  # If the person node has the label associated with either Actor or Director do ...

  if_is(Actor, Director) do
    def favorite_tv_show
      puts "3rd rock from the sun"
    end
  end
end

module User
  include Neo4j::ActiveModule

  property :login    
  property :password    
  property :roles    

  def stats
    puts "the stats"
  end
end

module Actor
  include Neo4j::ActiveModule
  include Showbusiness
end

module Director
  include Neo4j::ActiveModule
  include Showbusiness
end

module Showbusiness
  extend ActiveSupport::Concern

  included do
    property :biography    
    property :lastModified    
    property :version    
  end
end

standard_person = Person.new(name: 'John')
standard_person.name #=> John
standard_person.responds_to? :biography #=> false
standard_person.actor? #=> false

actor = Person.actor.new(name: 'Sam', biography: 'cool')
actor.actor? #=> true
actor.director? #=> false
actor.favorite_tv_show #=> "3rd rock from the sun"

director_actor = Person.director.actor.new(biography: 'stuff')
director_actor.actor? #=> true
director_actor.director? #=> true
director_actor.user? #=> false
director_actor.respond_to? :admin_powers #=> false
director_actor.mapped_label_names #=> [:Person, :Actor, :Director]

So in the above we can see a few things going on. We have a base Person class. This person class includes User, Actor, and Director ActiveModules. These ActiveModules are associated with a label of the same name. If a :Person node is retrieved from the database, and it includes a label associated with one of the ActiveModules, lets say the User module, then the node gets wrapped as a Person object but gains the properties associated with a User. User properties can be defined in the User module. They can also be defined in a special if_is(User) do block inside the Person class. This way, if a User module is included in several classes, those classes all gain the properties associated with the User modules. Additionally, a specific class could say that it's users gain properties / methods that other classes don't get.

At this point, I should also mention that I'm not committed to any of the naming / DSL, but hopefully you can see what I'm going for here.

Things like

module Animal
  include Neo4j::ActiveModule
end

has_many :out, :pets, type: :PET, model_class: :Animal

If you wanted a class to always have a ActiveModule's label / property, you could

class Person
  include Neo4j::ActiveNode
  include User
  include Actor

  by_default_has :User
end
@jorroll
Copy link
Contributor Author

jorroll commented Dec 15, 2017

I think I'd change the ActiveModule DSL to specifically use that of ActiveSupport::Concern i.e.

module User
  include Neo4j::ActiveModule

  def stats
    puts "the stats"
  end

  included do
    property :login    
    property :password    
    property :roles
  end
end

@jorroll
Copy link
Contributor Author

jorroll commented Dec 15, 2017

@leehericks, what do you think? ^^^

Seperately, regarding:

If you wanted a class to always have a ActiveModule's label / property, you could add something like by_default_has :User

Your ~ label :User, default: true is probably better. It still doesn't feel 100% right to me though, as, currently in ActiveNode, similar method calls (i.e. property :name, has_one :person, has_many :people) all add something to the model. In the label :User case (at least in my DSL) it's configuring something on the model.

@leehericks
Copy link

First, I don’t know why you opened another issue. There are enough open issues in this project and it cuts off a whole part of the discussion.

Second, if_is is waaaaaaay too much domain specific to your modeled domain example.

labels are concrete things added to the node, same as properties and associations. The functionality that can be associated with it is the optional domain-agnostic part

@leehericks
Copy link

I want to follow up that you are full of great ideas too and I’m just saying the framework should sit under you and allow you to design an if_is for your case. We just have to be sure what gets created and supported at the core is flexible and useful for everyone. I think that is why Brian introduced his thoughts with hesitation and the note that he wants to take his time on it. If we keep having good discourse we’ll nail something awesome.

Also you raising the issue of the movie database is a great real world example! It’s definitely a situation where inheritance is not the answer. Gonna dwell on it!!!

@cheerfulstoic
Copy link
Contributor

I'm fine with another issue, the other one was feeling a bit overwhelming ;). I'm just happy if we can agree on a proposal ;). I haven't read the other thread yet and I'll need to get back to work after this comment, but I thought I would start with this issue.

What you're suggesting @thefliik is, I think, closer to what I was thinking. Some feedback:

Regarding ActiveModule, I actually like ActiveLabel because it works at the same level as ActiveNode / ActiveRel (it describes the concept that it matches up with from the database).

Totally makes sense to have included in the modules based on ActiveSupport::Concern. The thing I would say about if_is(), though, is that part of the point of modules in Ruby is to have a centralized place to define behavioral functions, so I think that that method would belong inside of the module. But, of course, you don't want to have the function there if the label isn't attached to the node, so it should probably be inside of a block so that the methods only get attached to the object if the label is there on the node in the DB. I think that might look like:

module User
  include Neo4j::ActiveLabel

  def stats
    puts "the stats"
  end

  module InstanceMethods
    def admin_powers(id)
      Person.find(id).destroy
    end
  end

  included do
    property :login    
    property :password    
    property :roles
  end
end

To me this also highlights the question of the difference between the stats method and the admin_powers method. Since you're using include User inside of the Person model, that means that the Stats method will be defined on Person objects regardless of if the User label was there or not. Was that intentional? Also, if we delegated the included block directly to ActiveSupport::Concern I think that the same problem would occur (the block would be run at the point that the include User line is run, thus always defining those properties). I think it would be pretty easy, though, to have ActiveLabel's implementation of included only conditionally run the block based on if the label was there.

All of that said, I like the idea from the other thread about calling some method and giving the module so that the entire module will only be included depending on what things are defined. Something like:

class Person
  include Neo4j::ActiveNode

  label :User
end

This also gives us the power to have a single method with which to implement both cases we've been talking about: One where the label is always there (the default: true case) and the case where the label is only sometimes there (I might actually specify the option conditional: true or optional: true instead of the default: true in the other case).

I think that this would also give us the ability to say director.user? as well. If you just did include User, then you would only be able to have that return true in the case that it was included or it would raise a "method not found" error if it wasn't included. Probably the user? methods would only be defined in the optional: true case, though, because otherwise it would always return true and there's not really a point to it.

@jorroll
Copy link
Contributor Author

jorroll commented Dec 15, 2017

Regarding

  1. I like the idea from the other thread about calling some method and giving the module so that the entire module will only be included depending on what things are defined. label :User

Ah, so I didn't quite see it before, but I approached this from a slightly different viewpoint. My thinking with the above was that you include a module in a class and with that module comes a label. Your thinking seems to be, you include a label in a class, and with that label comes functionality. The difference between the two approaches is that, in your approach (I think), every ActiveNode in the app gains the functionality associated with a label if the label is present on the node. In my version, an ActiveNode only gains the functionality associated with a label if the module is included in the class and the label is present on the node. Put another way, you're adding powers to labels, whereas I was adding powers to modules. I'm a little hesitant about working with globals, but I can see how your approach makes sense for Neo4j. It certainly forces developers to always be thoughtful about their use of labels. I suppose in my scenario, for a (probably legacy) app, you could create two different ActiveLabel modules that both responded to the same label, but include them in different classes. Meaning that the different classes responded to the same label in fundamentally different ways. (which would make sense in this scenario, because devs wouldn't look to labels to see what functionality their classes had, they'd look to the included modules)

  1. Regarding ActiveModule, I actually like ActiveLabel because it works at the same level as ActiveNode / ActiveRel (it describes the concept that it matches up with from the database).

Makes sense, especially given my new understanding of your approach 👍 .

  1. The thing I would say about if_is(), though, is that part of the point of modules in Ruby is to have a centralized place to define behavioral functions, so I think that that method would belong inside of the module. But, of course, you don't want to have the function there if the label isn't attached to the node, so it should probably be inside of a block so that the methods only get attached to the object if the label is there on the node in the DB.

This thought makes sense given #1. In my DSL, you would need to include the User module in Person to gain its properties. In this scenario, an if_is() block inside Person makes sense (I think) because the person class is defining everything that applies to it. i.e. the Person class gets the User module, conditionally the Person class gets some methods if the User module is active. etc. In your scenario though, the User activelabel is independent from the Person class. I guess my last thought on this is that, conceptually, the "if this label is on a Person do this block" doesn't belong to the User Activelabel any more than it belongs to the Person Activenode. I'm fine with choosing to put it inside the module. Thinking about this more, in your DSL, I'd still advocate for putting this conditional block inside the class. In your scenerio, the ActiveLabel is truly global whereas the class already has at least one conditional (i.e. it might have an additional label). I'd say, keep the conditionals in the class (i.e. it might have an additional label, and if it does do this).

Update: actually I forgot that a label could say something like "if this is included in the Person class OR if this is included in the Car class, do this". Code like that would definitely be better placed in the module, so it makes sense to put all conditional code there.

  1. To me this also highlights the question of the difference between the stats method and the admin_powers method. Since you're using include User inside of the Person model, that means that the Stats method will be defined on Person objects regardless of if the User label was there or not.

So in my DSL, I had an unstated, subconscious, expectation that anything inside an ActiveLabel module only applies if it is included inside an ActiveNode AND the node has the label associated with the ActiveModule. If you wanted to always include functionality in a class, you'd need to make a separate concern. So in the above, any class that inherits the User module gains the stats method only if the node also has a :User label. And only the person class gains the admin_powers method if a person node also has a :User label.

Fleshing my subconscious expectations out further, including a User ActiveLabel in a class would always add some general methods, such as the ability to call .user? on any instance of that class, or the ability to call a .user singleton method to create a new person with a user label (i.e. Person.user.new)

  1. I think that this would also give us the ability to say director.user? as well. If you just did include User, then you would only be able to have that return true in the case that it was included or it would raise a "method not found" error if it wasn't included.

This was intentional on my part, I was thinking that you would only want the ability to call .user? on classes that you explicitly included the User module in. Are you thinking that every ActiveNode in the app would gain the ability to call .user? I imagine you'd only have the ability to call .user? on classes that called label :User.

@jorroll jorroll changed the title feature: an ActiveLabel / ActiveModule module for sharing ActiveNode logic feature: an ActiveLabel module for sharing ActiveNode logic Dec 15, 2017
@jorroll
Copy link
Contributor Author

jorroll commented Dec 16, 2017

So if my understanding of your ActiveLabel concept is correct, and any node with the ActiveLabel's label will gain the ActiveLabel's properties, I've thought of an issue in my app:

In my app, I track node state through :Current and :Destroyed labels (you can learn more here). Supporting this are a set of custom CRUD methods. I also want to track relationship state, but Neo4j doesn't support multiple labels on a relation, so I model stateful relations using two relations with a node in the middle: (person)-[:FRIEND]->(:FriendRel:Current)-[:FRIEND]->(person) To mark the relation as destroyed: (person)-[:FRIEND]->(:FriendRel:Destroyed)-[:FRIEND]->(person). Tracking relations also makes use of :Current / :Destroyed labels, but the associated CRUD operations (i.e. ActiveLabel methods / properties) are completely different. The ActiveLabel scheme I proposed supports this, I could create two different ActiveLabel modules, both of which are associated with :Current labels, but do different things. Classes representing a trackable relation would include one version of the ActiveLabel module, classes representing trackable nodes would include a different version of the ActiveLabel module.

Do you envision this use case being supported in your ActiveLabel scheme? How?

@jorroll
Copy link
Contributor Author

jorroll commented Dec 16, 2017

To pile on a bit more, I can see how my scheme could be extended to create a module that only applies if TWO labels are attached to a class. Something like this, highly contrived, example:

class Animal
  include Actor
  include Living
  include Extraordinary

end

class Person
  include Actor
  include Living
end

Where the Extraordinary module was associated with BOTH Living and Actor labels but would only "activate" if both labels were present on an Animal.

Animal.actor.living.new
  # same as
Animal.extraordinary.new

Now thats some polymorphism :)

@leehericks
Copy link

@thefliik Regarding tracking relationships, I think you're missing a very simple model.

(p1:Person)-[:FRIENDSHIP]->(p2:Person)
(p1:Person)-[:DESTROYED_FRIENDSHIP]->(p2:Person)

This article from Neo4j Blog:
https://neo4j.com/blog/neo4j-genericvague-relationship-names/

As much as we want to live in an ideal beautiful data modeling world, Neo4j has performance quirks to force us to optimize.

class Person
  include Neo4j::ActiveNode

  has_many :both, :friends, type: :FRIENDSHIP, model_class: :Person
  has_many :both, :destroyed_friends, type: :DESTROYED_FRIENDSHIP, model_class: :Person 
  # Might use ActiveRel for timestamps

  # Add logic for destroying friendships
end

@leehericks
Copy link

To pile on a bit more, I can see how my scheme could be extended to create a module that only applies if TWO labels are attached to a class.

before_save, if labels contains :A and :B then do something.
or define a method that checks the existence of the two labels and returns boolean and use it around your code. Something like this that transcends the duties of one label should probably be put in a module and included, being separate from the things that one label adds.

@leehericks
Copy link

Regarding the movie database, whether labeled an Actor, Director, or some combination thereof, first and foremost the nodes have a shared base label. They are a Person, but they might be multiple things. This is where inheritance fails.

This is also why the things granted by the existence of a label can't be conditionally granted in terms of adding removing model functionality and one has to be careful about clashing names.

A director has movies they directed
An actor has movies they acted in

The associations of each label can't all be named movies.

This is why I caution that we add the ActiveLabel feature and you make a module to share which checks for the existence of multiple labels separately. That's a Ruby language feature that doesn't need any special Neo4j.rb additions.

@cheerfulstoic
Copy link
Contributor

I was also going to suggest either the FRIENDSHIP / DESTROYED_FRIENDSHIP solution or having a destroyed property on the relationships. Both have performance indications, but both would probably perform better than two relationships and a node (though on a small to mediumish scale it probably doesn't matter).

Regarding the last thing that you said @thefliik about polymorphism, it would be super cool for ActiveLabel modules to automatically add scopes. I would have a where_ prefix for clarity, so:

Animal.where_actor.where_living.new

Ah, so I didn't quite see it before, but I approached this from a slightly different viewpoint. My thinking with the above was that you include a module in a class and with that module comes a label. Your thinking seems to be, you include a label in a class, and with that label comes functionality. The difference between the two approaches is that, in your approach (I think), every ActiveNode in the app gains the functionality associated with a label if the label is present on the node. In my version, an ActiveNode only gains the functionality associated with a label if the module is included in the class and the label is present on the node.

I think we're getting close, but not quite. I'm not suggesting that models ever get the behavior from a module if that module wasn't explicitly defined on the model. I think it definitely makes sense that the designer of the app needs to call out specifically on what models an ActiveLabel module can affect behavior of objects of that model class. So I think we're on the same page there.

Where I'm coming from is that I think that I'm seeing two different use cases for ActiveLabel modules:

  1. Nodes of the model sometimes have that label, and thus should "act" like that ActiveLabel module when the label is applied
  2. Nodes will ALWAYS act like the ActiveLabel module and the label will always be applied on nodes created for that module.

I think that Destroyed is a good example of the first use case.

I see the second case as basically multiple inheritance (which is really the point of modules in Ruby anyway). It would be as if the class had more than one parent class, meaning that the objects would always have the labels for the modules and searching on the module like TheModule.all would give you a proper superset of all the nodes for the classes that have that module (because, again, the label would always be there).

I could say more, but since we have a couple of threads going I wanted to make sure we're on the same page with this point first.

@leehericks
Copy link

leehericks commented Dec 16, 2017

@cheerfulstoic @thefliik
Let's first decide on these core API which sit under other features such as ActiveLabel. With these core API in place, anyone can easily mixing and add functionality to their models based on node labels.

ActiveNode:

# Fixed model labels
# Deprecate self.mapped_label_name, setting it calls self.model_labels = [mapped_label_name]
Model.model_labels=([:Label1, :Label2])

# Synthesizing optional labels
Model.optional_label(name, options)

# Manipulating labels
model.labels     # Array of label names which includes ActiveNode model/inheritance labels and applied optional labels

# Querying
model.has_labels([:Label1, :Label2])
model.where(has_labels: [:Label1, :Label2])

optional_label Options

  • default:Boolean [default false, when true label is added to new instances]
    The default option is a boolean because it matches the synthesized method switching the label on and off.

Calling optional_label synthesizes:

# Add or remove the label
model.labelled_[name]=(Boolean)

# Check Existence
model.labelled_[name]?    # Returns boolean

# Querying
model.labelled_[name]      # Query scope, Person.labelled_actor.labelled_director

Labels should get persisted on call to save to allow validations and custom logic in callbacks.

Example:

class Case
  include Neo4j::ActiveNode
  optional_label :Current, default: true
  property :title, type: String

  def archive
    labelled_current = false
  end
end

Case.all.labelled_current

@leehericks
Copy link

Remember that learning a framework is hard and this is on top of Rails, Ruby language, understanding Neo4j and how to data model for the graph, etc. I'm not a genius and I struggle a lot. Ambiguous naming can be really confusing. Clear context is everything.

@leehericks
Copy link

Well, I see add_labels, remove_labels, mapped_label_names, etc. already exist. ^^;

@cheerfulstoic
Copy link
Contributor

I could respond, but I'd like to try something that I hope might be more effective. One idea that I've had for developing something in the past which I haven't really tried is "documentation driven design". That is, before writing any code, write the documentation for how it would work so that you're thinking of it from the user's perspective (kind of what @leehericks is doing). Once you're happy with that you can write the code and you should have really good documentation for it.

So what about this: We have a PR which defines documentation for non-existent code. This way we can discuss things in the comments, but we can also comment on specific lines / sections via GitHub's awesome PR UI.

Worth a shot?

@leehericks
Copy link

Yes, I have no experience with github PRs, but can we all edit the example code or does one person initiate it and then update the documentation according to comments and consensus?

@leehericks
Copy link

@cheerfulstoic At some point as the lead developer with a deeper understanding of the framework you do have to comment. 🤪

I assume if you see something that is impossible or misunderstood, you'll let me know. I don't want to labor too long under misunderstandings.

@leehericks
Copy link

P.S. I am trying to dig into some of the source code/documentation to get better acquainted with the project. :)

@cheerfulstoic
Copy link
Contributor

Yeah, I definitely have comments, but I feel like we're three people pulling in three directions on a variety of issues and it's really hard to keep track of it.

With GitHub PRs, one person makes a branch/fork and submits a PR for the patch of those changes. PRs are just issues in GitHub, but you can also see the different proposed file changes and click on individual lines to start a conversation about that line (or the block of code above that line). There's more, but that's the main thing I wanted to take advantage of.

@thefliik ?

@leehericks
Copy link

So are we all going to create our own PR then? Like fork the project and add a markdown file or a ruby file and create a pull request for that?

@cheerfulstoic
Copy link
Contributor

I was thinking of having one person create a PR and we can all comment on it (and maybe allow changing it too for efficiency since we're sort of spread out across the world). But we could also do separate PRs and then we could each comment on each others' and probably solidify on one. It would certainly be a nice way to be able to show where we are the same and different.

Regarding what we would create, this repo has a docs directory which is where all of the documentation is kept. The files are in reStructedText format. I chose that because it's the default choice of readthedocs. RTD supports markdown, but it doesn't provide many features in that context (RTD uses Sphinx, a popular tool in the python community, to generate a documentation suite). So a PR would probably just need to include a new docs/ActiveLabel.rst file (along with the corresponding change to the table of contents at the top of docs/index.rst).

@cheerfulstoic
Copy link
Contributor

Ok, I've put together a bit of documentation:

#1457

Definitely not complete! Definitely didn't get everything in there! Just trying to get a start on something where we can be more focused on how we have a conversation.

@cheerfulstoic
Copy link
Contributor

We can discuss just on there or we can have other proposals too. Happy to go in whatever direction brings clarity

@leehericks
Copy link

Thank you Brian! Sorry, currently having a school IT crisis. FML lol

@cheerfulstoic
Copy link
Contributor

No problem, I'll be busy in the upcoming days, so take your time ;)

@jorroll
Copy link
Contributor Author

jorroll commented Dec 20, 2017

H'okey, back. Ahhh...again I find myself wishing that Github allowed nested replies.

Regarding

@thefliik Regarding tracking relationships, I think you're missing a very simple model.
(p1:Person)-[:FRIENDSHIP]->(p2:Person)
(p1:Person)-[:DESTROYED_FRIENDSHIP]->(p2:Person)

You're totally right. Until I read that GraphAware article you shared, @leehericks, I don't think I realized how expensive filtering on a label was. I'm definitely going to update my tracked relations with _CURRENT or _DESTROYED suffixes (It's important for me to call out _CURRENT relations as well, because it distinguishes between a tracked / untracked relation). Unfortunately, this does not let me rid myself of the general (person)-[:FRIEND]->(:FriendRel:Current)-[:FRIEND]->(person) format, as I also track which user created the relation, which necessitates a (user)-[:CREATED]->(:FriendRel). Still, for most queries this will allow (person)-[:FRIEND_CURRENT*2]->(person) which looks like it will provide a nice speedup (and also simplifies the query). Separately, I'll need to think about if I need (:FriendRel:Current) or if I can get away with just (:FriendRel).

Regarding

To pile on a bit more, I can see how my scheme could be extended to create a module that only applies if TWO labels are attached to a class.

before_save, if labels contains :A and :B then do something.
or define a method that checks the existence of the two labels and returns boolean and use it around your code. Something like this that transcends the duties of one label should probably be put in a module and included, being separate from the things that one label adds.

Maybe you know of a Ruby secret I don't, but conditionally adding a module to an instance of a class isn't as easy as you imply. Ya, testing for the condition is easy, but how do you include the module? I can use obj.extend to include the method definitions, I'd need to, seperately, evaluate the included block. (And currently, Neo4jrb doesn't add a property method to instances of a class). This is the whole point of this conversation (for me at least). And if you do know of some easy way of accomplishing this, then I might agree that the gem should stay as is and we can just update the documentation. As it stands, I think there are some steps the gem could take to make dev's lives easier.

Let's first decide on these core API which sit under other features such as ActiveLabel.

Well, I see add_labels, remove_labels, mapped_label_names, etc. already exist. ^^;

Ya

@cheerfulstoic

Yeah, I definitely have comments, but I feel like we're three people pulling in three directions on a variety of issues and it's really hard to keep track of it.

I agree that we are currently three people pulling in different directions. Jumping over to look at the documentation though, I see some assumptions there that I disagree with. Most importantly, I'm still arguing that, at the core, what an ActiveLabel should do is add stuff to an instance of a class if a specific label is associated with the instance. How that label is added to an instance of the class, in my mind, is a separate conversation (though it would certainly be present in the documentation).

A point against the documentation approach, is that it immediately mixes in all of these different concerns together in the conversation (e.g. your documentation touches on "how do we add additional mapped label names to a class?"). I like the idea of starting with the documentation, but, personally, I'd like to understand what ActiveLabel modules are trying to accomplish first.

My argument: ActiveLabel modules should add all their custom functionality if a specific label is present on an instance of the class. Otherwise, they should add none of the custom functionality. ActiveLabel modules should always add some singleton methods to the class (to be determined).

If folks agree with this, personally I can move forward, but if folks don't agree with this, I'd like to hear their thoughts on why not? Looking at the documentation, you've put together @cheerfulstoic, it appears that your ActiveLabels are trying to combine multiple ideas (i.e. some ActiveSupport::Concern, some additional_mapped_label_names, some conditionally adding custom behavior to a class instance if a label is present, etc). Currently, I'm not in favor of this approach, though maybe it's simply because I don't understand your reasoning.

If your approach is the same as mine, then label :Destroyable, optional: true doesn't seem necessary. When you include the ActiveLabel module in the class, that already implies that the :Destroyable label is optional. You'd only need some kind of label :Destroyable dsl if you wanted to specify that :Destroyable was an additional_mapped_label_name (i.e. always present).

Alternatively, if you're thinking of the label :Destroyable, optional: true api as a way to simply include an ActiveLabel module, that's (obviously) really confusing for me (because it looks like you're trying to describe what labels are present on the class). If you want to include a module in a class, Ruby already has the include api for this.

class Person
  include Destroyable
  # Developer opens the destroyable module, sees that it's an `ActiveLabel` module,
  # and understands that Person nodes may have a `:Destroyable` label
end

A developer could add an optional label to a class that isn't associated with any ActiveLabel functionality, after all.

Reflection

When I opened this thread, I made the mistake of mixing in a number of separate ideas based on unspoken assumptions I had. I should have started more focused, waited for thoughts, and moved out from there.

@jorroll
Copy link
Contributor Author

jorroll commented Dec 20, 2017

Looking some more, it really looks the label :SomeLabel API is a combination of "add some label" with "add this module if this label is present." My vote is to separate those two resulting in something like

If the additional label is always present

class Person
  include Neo4j::ActiveNode
  include Destroyable

  add_label :Destroyed
end

# OR

class Person
  include Neo4j::ActiveNode
  include Destroyable

  add_label Destroyable.follows_label # .follows_label would return whatever label the `ActiveLabel` module is associated with
end

# OR using #1414

class Person
  include Neo4j::ActiveNode
  include Destroyable

  self.additional_mapped_label_names = [:Destroyed]
end

If the additional label is optional

class Person
  include Neo4j::ActiveNode
  include Destroyable
end

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

No branches or pull requests

3 participants