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

Adding Atom feed #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding Atom feed #59

wants to merge 1 commit into from

Conversation

Lucas-C
Copy link

@Lucas-C Lucas-C commented Feb 21, 2015

Adresses #58

The feed has been validated using feedvalidator.org and validator.w3.org/feed/check.cgi

Note that this is the very first time I'm coding in Ruby ^^
I'm open to every kind of feedback: code style, idioms, separation of concerns in Rails...

@@ -7,6 +7,8 @@

resources :places, only: [:new, :create, :edit, :update]

get "/feed" => "places#feed", :as => "feed", :defaults => { :format => 'atom' }

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.
Prefer single-quoted strings when you don't need string interpolation or special symbols.

@Lucas-C Lucas-C force-pushed the master branch 6 times, most recently from 5e05821 to fe74084 Compare February 21, 2015 15:33
@Lucas-C
Copy link
Author

Lucas-C commented Feb 21, 2015

What it looks like:

techlist_feed

@Lucas-C Lucas-C force-pushed the master branch 3 times, most recently from 264dedc to 50ce003 Compare February 23, 2015 11:17

feature 'A user subscribe to the place Atom feed' do
scenario 'and consult the complete correct feed page' do
CsvImportService.new('Place', Rails.root.join('spec/fixtures/csv/valid.csv'), {state: 'active'}).save

Choose a reason for hiding this comment

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

Redundant curly braces around a hash parameter.
Space inside { missing.
Space inside } missing.

@Lucas-C Lucas-C force-pushed the master branch 2 times, most recently from 786c186 to 4cbb773 Compare February 23, 2015 11:20
@scharrier
Copy link
Member

Thx ! I'll try to find some time to review it during the week.

@scharrier
Copy link
Member

Sorry for the delay. First, thanks a lot for your contribution !

I though about how we should add this atom feed, and I think the API would be a better place, even if it can be a surprising choice. Data presented in the feed is exactly the same that in the places endpoint (api/v1/places.json), except that it's not the same format, and places aren't ordered on the same attribute. With this in mind, it totally makes sense (for me, at least), to use respond_to in controllers/api/v1/places#index to handle the Atom response.

We would then be able to have /api/v1/places.json and /api/v1/places.atom endpoints, which could evolve like the rest of the API.

Thoughts ? ping @Lucas-C

@@ -2,6 +2,12 @@ class PlacesController < ApplicationController
def index
end

def feed
Copy link
Member

Choose a reason for hiding this comment

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

See #59 (comment). Could be moved to a respond_to in api/v1/places#index.

@@ -1,6 +1,6 @@
FactoryGirl.define do
factory :place do
name 'Craftsmen Angers'
sequence(:name) { |n| 'Craftsmen Angers #{n}' }

Choose a reason for hiding this comment

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

Unused block argument - n. You can omit the argument if you don't care about it.

@Lucas-C
Copy link
Author

Lucas-C commented Aug 9, 2015

My turn to be late to answer :) Sorry !

Ok, except from your comment on not using a function in a view, I think I addressed all the points you raised.
That is essentially moving the atom feed management into the controller api namespace and using FactoryGirl to improve the tests.

For the remaining issue with the function definition in the view:

  • why is that a problem ? It is only doing output formatting
  • adding a presenter class or defining an equivalent partial function in the model looks like bloat code to me. Do you have any other suggestion ?

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.

None yet

3 participants