-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: master
Are you sure you want to change the base?
Adding Atom feed #59
Conversation
@@ -7,6 +7,8 @@ | |||
|
|||
resources :places, only: [:new, :create, :edit, :update] | |||
|
|||
get "/feed" => "places#feed", :as => "feed", :defaults => { :format => 'atom' } |
There was a problem hiding this comment.
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.
5e05821
to
fe74084
Compare
264dedc
to
50ce003
Compare
|
||
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 |
There was a problem hiding this comment.
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.
786c186
to
4cbb773
Compare
Thx ! I'll try to find some time to review it during the week. |
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 ( We would then be able to have Thoughts ? ping @Lucas-C |
@@ -2,6 +2,12 @@ class PlacesController < ApplicationController | |||
def index | |||
end | |||
|
|||
def feed |
There was a problem hiding this comment.
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}' } |
There was a problem hiding this comment.
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.
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. For the remaining issue with the function definition in the view:
|
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...