Skip to content

Instantly share code, notes, and snippets.

@YayC

YayC/migrating_sinatra.md

Created April 9, 2013 19:16
Show Gist options
  • Save YayC/5348527 to your computer and use it in GitHub Desktop.
Save YayC/5348527 to your computer and use it in GitHub Desktop.
Download ZIP
migrating sinatra repo link
@edshadi
Copy link

edshadi commented Apr 9, 2013

your app doesn't run, so it was hard for me to do proper review, here are some findings:

  • Migration doesn't run without running a manual postgres restore and the dump file is not even included.
  • I couldn't find a way to populate the categories or tags tables.
  • I'm not sure what the seed file is supposed to be doing.
  • I think the new url is easier done through nested routes rather than a plain match.
  • i don't see a reason for the old_url action, couldn't we just handle all cases through show
  • the url column in articles table has too much information, also if I create a new article, how does it get set?
  • don't user "/categories/#{category.content}/articles/#{article_title}", user named routes, it's preferred. You can name a route by using :as => route_name
  • what happens if I change the article's title? you won't be able to find it with the old urls anymore.
  • this app is pretty much untested.
  • it's hard for me to do a thorough code review since I can't get the app to work.

Please go back and revisit the app, make sure it works, work on the above comments and resubmit for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment