Add a new BaseResource model that will be used by all API models.

Review Request #3785 — Created Jan. 21, 2013 and submitted

Information

Review Board
master

Reviewers

Add a new BaseResource model that will be used by all API models.

The BaseResource model provides all the common functionality for dealing
with requests and parsing results, ensuring a model is ready to use (has
its data fetched) before we do anything with it, and being able to
ensure it's created before using it. These models have a concept of a
parent resource that they're under (for example, a diff being under a
review draft).

This change just introduces this model and a suite of unit tests for it.
We'll move over to this in subsequent commits.
Wrote unit tests to cover all the various code paths, testing for success,
failure, possible things that could be overridden, and so on. They all pass.

This was developed along with a change to migrate DiffComment and use that,
and that change worked. That'll be up once the other Comment classes have been
converted.
Description From Last Updated

Shouldn't this var be defined at the top of the function?

daviddavid

Same with these?

daviddavid
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/js/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/baseResourceModel.js
    
    
  2. 
      
david
  1. 
      
  2. Shouldn't this var be defined at the top of the function?
    1. No, they don't have to be at the top of the function. Just the current scope level.
    2. I thought that "var" always had function scope, and the point of defining it at the top of the function was to ensure that was clear. "let" is the block-scoped version, which we probably can't use yet.
    3. Yeah, you're right. I guess I thought it was at the top of the block scope.
      
      There's a tradeoff though in readability, I feel, with keeping them at the function scope. And from the various conversations I'm reading on this, it's very clearly not a black-and-white issue.
      
      I went to look at other projects to see what they did. I looked at jQuery, Backbone.js, Underscore.js, Knockout.js, and Angular.js, which are all pretty well-used frameworks. None of them enforced function-level-only declarations, which I found interesting. They seem to opt for readability, keeping the vars closer to where they're used (some putting them wherever they want, some putting them at the top of the block).
      
      There's a tradeoff. We can follow the "let's put everything at the top of the function" model, which is more accurately representative of what JS does under the hood, but doesn't quite keep code conceptually together as well. Or, we can keep it at the block level, which does mean the declarations (note: *NOT* the initial values) will be hoisted to the top of the function. I guess it depends on how close we want to follow the underlying behavior vs. how readable we want the code to be for larger functions.
      
      I sort of feel that keeping the vars on the highest level in which they're used is the way to go, but I'm also a bit biased since that's how I've been developing for so long. The only time in which this appears to be able to bite you in practice is if you try redeclaring a var in an inner block and expecting it to not affect the outer block when it's used next, like:
      
          var i = 10;
      
          if (foo) {
              var i = 20;
          }
      
      In this case, after the if statement, 'i' will be 20, not 10. However, redeclaring a variable is something I feel should be avoided in any language anyway.
      
      Only the declarations are hoisted, not the initial values. So if the first 'i' above wasn't defined, and you immediately tried to console.log it, it would be undefined, not 20.
      
      What are your thoughts?
    4. Given that we'll probably turn on jslint in Review Bot, I think we should probably move the var declarations to always be at the top of the function. It's fine to keep the initializers nested, especially in cases where they depend on previous code. I don't really like it, but I feel like the whole "var" thing is pretty crappy no matter what.
    5. Regardless of this, I think we should use JSHint and not JSLint. I'm hearing a lot of good about JSHint these days (it's less strict about things that are more Crockford's personal preferences and not JavaScript things). Interestingly, it doesn't enforce the function-level vars.
      
      I'll go ahead and update the code.
  3. reviewboard/static/rb/js/models/baseResourceModel.js (Diff revision 1)
     
     
     
     
     
     
    Same with these?
  4. 
      
chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
      Ignored Files:
        reviewboard/static/rb/js/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/baseResourceModel.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (d6b8ef75e73b2d2a8351a3cbde686c949b6d1733)
Loading...