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

Move buildURL and related methods to a mixin #2812

Merged
merged 3 commits into from
Mar 13, 2015

Conversation

amiel
Copy link
Contributor

@amiel amiel commented Feb 21, 2015

This is my first step towards emberjs/rfcs#4.

The intent here is to separate url building from the RESTAdapter so that
it can eventually be included in other adapters and hold the logic for the
path template concept described in emberjs/rfcs#4.

Even though it's a fairly small change, I've written this pull request to:

  1. Get my foot in the door. Since this is my first pull request to ember-data, I figure starting small is a good idea.
  2. Make a place for the url-templates feature before starting a longer-lived branch. I think this will help prevent conflict management overhead as I work on the url-templates feature.

@fivetanley
Copy link
Member

This seems great to me! I wonder how this will affect the website. I think we still want the methods to show up on the documentation for DS.Model.

Do you have ruby installed? If so can you try building this in the website repo and post some screenshots of the doc output?

@amiel
Copy link
Contributor Author

amiel commented Feb 25, 2015

I do have Ruby installed, but have never generated the API docs before. I'll try following the instructions at https://github.com/emberjs/website/blob/master/README.md and post the results.

@bmac
Copy link
Member

bmac commented Feb 26, 2015

Should we be exporting the BuildUrlMixin on the DS global?

@bmac
Copy link
Member

bmac commented Feb 26, 2015

@amiel I suspect @fivetanley is correct and the buildURL and urlPrefix will not show up on the RESTAdapter class in the docs. I you will likely need to define a BuildUrlMixin for your new mixin and use the YUIDoc @uses syntax to show that the RESTAdapter mixes in the BuildUrlMixin's methods.

@amiel
Copy link
Contributor Author

amiel commented Feb 26, 2015

Hmmm, I can't get the docs to generate:

$ pwd
/Users/amiel/src/github/ember/website
$ bundle exec rake generate_docs
Generating docs data from /Users/amiel/src/github/ember/ember.js... Could not locate Gemfile or .bundle/ directory
You are missing dependencies for /Users/amiel/src/github/ember/ember.js. Attempting to install now.
bundle install
Could not locate Gemfile or .bundle/ directory
rake aborted!

Should there be a Gemfile in the ember.js repo?

I'm using ruby 2.1.0.

This is my first step towards emberjs/rfcs#4.

The intent here is to separate url building from the RESTAdapter so that
it can eventually be included in other adapters and hold the logic for the
path templating described in emberjs/rfcs#4.
@amiel amiel force-pushed the move-buildUrl-to-mixin branch from 804e431 to 6835936 Compare February 26, 2015 23:51
@amiel
Copy link
Contributor Author

amiel commented Feb 27, 2015

@bmac @fivetanley I've tried for a while to generate the API docs to verify. Since I couldn't get anything working, I've just made the changes you suggested.

I wasn't going to export BuildURLMixin on the DS global initially, but if it helps the docs, I'm all for it.

@@ -0,0 +1,111 @@
var get = Ember.get;

export default Ember.Mixin.create({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a doc block at the top with an @class BuildURLMixin annotation for the methods below to get correctly attributed to the correct class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmac ok, I've added this, and some documentation for how to use BuildURLMixin. It is not very thorough, but my goal is to completely change the way url building is done (with url templates), and I don't want to spend too much time improving documentation for something that will soon be obsolete.

@bmac bmac added this to the 1.0.0-beta.16 milestone Feb 28, 2015
@amiel amiel force-pushed the move-buildUrl-to-mixin branch from 774bb10 to 31faddd Compare March 2, 2015 22:17
@amiel amiel force-pushed the move-buildUrl-to-mixin branch from 31faddd to 486f768 Compare March 4, 2015 17:59
@amiel
Copy link
Contributor Author

amiel commented Mar 4, 2015

Ok, I've got the docs building now, thanks @rwjblue.

Here's what it looks like now:

screen shot 2015-03-04 at 10 02 03 am

screen shot 2015-03-04 at 10 01 34 am

fivetanley added a commit that referenced this pull request Mar 13, 2015
Move buildURL and related methods to a mixin
@fivetanley fivetanley merged commit 6449e67 into emberjs:master Mar 13, 2015
@amiel amiel deleted the move-buildUrl-to-mixin branch March 13, 2015 17:54
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

Successfully merging this pull request may close these issues.

4 participants