Skip to content

Adding Atom feed#59

Open
Lucas-C wants to merge 1 commit intomainelabs:masterfrom
Lucas-C:master
Open

Adding Atom feed#59
Lucas-C wants to merge 1 commit intomainelabs:masterfrom
Lucas-C:master

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...

config/routes.rb Outdated

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

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

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.

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.

3 participants