Add javascript models for Repository, Branch, and Commit.

Review Request #4266 — Created June 26, 2013 and submitted

Information

Review Board
master

Reviewers

Add javascript models for Repository, Branch, and Commit.

This change adds models and collections for repositories, branches, and commits,
along with basic tests for the fetching and parsing logic. This implements the
client side of the new commits and branches resources.
Ran unit tests. Used these models in my larger post-commit changes.
Description From Last Updated

If these are instance variables, you don't want them here. They should be set in initialize(). Every object should specifically …

chipx86chipx86

This is just testing that we set it above. Doesn't seem to be very useful as-is.

chipx86chipx86

Probably, we should just use options.blah instead of setting these values. Then you'd define: collection = new RB.RepositoryCommits([], { urlBase: …

chipx86chipx86

Did you mean to leave this?

chipx86chipx86

Can you wrap this?

chipx86chipx86

Every Model has an id field, so we don't need this. By setting id below, the model's ID will be …

chipx86chipx86

Missing ;

chipx86chipx86

I feel like this should be a Resource model. I don't know that it's really worth having inconsistent modelling for …

chipx86chipx86
chipx86
  1. 
      
  2. Show all issues
    If these are instance variables, you don't want them here. They should be set in initialize().
    
    Every object should specifically set every instance variable it will use in its lifetime in a default value in initialize(). Any object in JavaScript that dynamically defines new variables on itself post-initialize takes a performance and memory hit, since the VM can't share as much state. (There was a good writeup on this somewhere, but I can't find it off-hand.)
  3. Show all issues
    This is just testing that we set it above. Doesn't seem to be very useful as-is.
  4. Show all issues
    Probably, we should just use options.blah instead of setting these values. Then you'd define:
    
    collection = new RB.RepositoryCommits([], {
        urlBase: url,
        start: start
    });
    
    The collection would then use this.options.urlBase/start instead of this.urlBase/start, which is more Backbone-ish.
  5. Show all issues
    Did you mean to leave this?
  6. Show all issues
    Every Model has an id field, so we don't need this. By setting id below, the model's ID will be the provided ID. I think that's okay, but worth noting.
  7. Any risk of this failing?
    1. In the case where there's nowhere to split, it will just return a single-item array with the string itself, which matches the way we handle single-line commit messages in the perforce change description parsing.
  8. Show all issues
    Missing ;
  9. Show all issues
    I feel like this should be a Resource model. I don't know that it's really worth having inconsistent modelling for these things.
    
    I also want to start putting everything api-related in resources/{collections,models}/, so I think this and the other resource models/collections should go in there.
    1. I'm really kind of wary about pulling in all the complexity of BaseResource, since at the moment this will never load from or save to the server, and is really just a place to aggregate the repository ID, branches, and commits. Using BaseResource means that in order to get a url I have to set a parent object and deal with ready().
    2. I'm going to need it for Repository. The new smarter repository creator will need to load in resources and create them, so the complexity's going to be used there at least.
    3. If you want me to do the conversion at a later point, I can.
  10. 
      
david
david
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (20024e9).