Skip to content

Instantly share code, notes, and snippets.

@CarlosGabaldon

CarlosGabaldon/pr_review_phil_thompson

Last active August 29, 2015 14:03
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save CarlosGabaldon/37e3e04849a8e02a2513 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save CarlosGabaldon/37e3e04849a8e02a2513 to your computer and use it in GitHub Desktop.
Download ZIP

Revisions

  1. CarlosGabaldon revised this gist Jul 10, 2014. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion pr_review_phil_thompson
    View file
    Original file line number Diff line number Diff line change
    @@ -19,5 +19,5 @@ Story 4
    Overall
    - Clean implementation, most of the problems were solved with the most common expected ways
    - Easy to follow PR
    - Very details commit comments
    - Very detailed commit comments
    - Liked the notes on how future improvements could be made to the overall desgin of the publisher importer
  2. CarlosGabaldon created this gist Jul 10, 2014.
    23 changes: 23 additions & 0 deletions pr_review_phil_thompson
    View file
    Original file line number Diff line number Diff line change
    @@ -0,0 +1,23 @@
    Story 1
    - Liked the fix for the content-length warning
    - Solved the problem in common expected way with includes
    - Added good commit notes on changes, assumptions, and possible future enhancements

    Story 2
    - Liked the addition of the migration for production and update seed for dev
    - + for added deal_heler unit test

    Story 3
    - Used Timecop, which is the most common way to solve this time issue


    Story 4
    - Fairly clean approach
    - Extra points for integration and unit tests
    - Not sure why app/views/advertisers/index.html.erb was modified

    Overall
    - Clean implementation, most of the problems were solved with the most common expected ways
    - Easy to follow PR
    - Very details commit comments
    - Liked the notes on how future improvements could be made to the overall desgin of the publisher importer