Add path for downloading translations with a hierarchy instead of a flat #27

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@cmavromoustakos
Contributor

cmavromoustakos commented Mar 15, 2012

The reason for this change is that most javascript I18N plugins expect the data in the same hierarchy that it is stored on as the locale yaml files.

Currently, we have to come up with a conversion to store and retrieve our keys differently to match the key to the translation. With this change we allow the flexibility of allowing the end user (CopyCopter client or some other javascript client) to pass a optional argument formart with a type of hierarchy to get the data in the format of:

  {
    "en": {
      "test": {
        "one": "expected one",           
        "two": "expected two"
      }
    }
  }
Gemfile
@@ -24,10 +24,11 @@ end
group :test do
gem 'bourne', '1.1.1'
gem 'capybara-webkit', '0.3.0'
- gem 'cucumber-rails', '0.4.1'

This comment has been minimized.

Show comment Hide comment
@patricksrobertson

patricksrobertson Mar 15, 2012

Contributor

Please keep gem versions explicit.

@patricksrobertson

patricksrobertson Mar 15, 2012

Contributor

Please keep gem versions explicit.

.rvmrc
@@ -1 +1 @@
-rvm 1.9.2
+rvm use 1.9.2-p180

This comment has been minimized.

Show comment Hide comment
@patricksrobertson

patricksrobertson Mar 15, 2012

Contributor

1.9.2's current patchlevel is 315. Enforcing an old one may not be advisable.

@patricksrobertson

patricksrobertson Mar 15, 2012

Contributor

1.9.2's current patchlevel is 315. Enforcing an old one may not be advisable.

This comment has been minimized.

Show comment Hide comment
@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

This was removed by Dan on master and I will remove the file completely here also.

@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

This was removed by Dan on master and I will remove the file completely here also.

Gemfile
gem 'database_cleaner', '0.7.1'
gem 'factory_girl_rails', '1.7.0'
gem 'fakeweb', '1.3.0'
+ gem 'ruby-debug19', '~> 0.11.6'

This comment has been minimized.

Show comment Hide comment
@patricksrobertson

patricksrobertson Mar 15, 2012

Contributor

This is the same mention for locked version numbers.

@patricksrobertson

patricksrobertson Mar 15, 2012

Contributor

This is the same mention for locked version numbers.

This comment has been minimized.

Show comment Hide comment
@croaky

croaky Mar 15, 2012

Contributor

Is there a way to use Ruby debug without including it in the repo? I've been on a 'try not to include anything machine- or developer-specific' kick lately.

copycopter/style-guide@1bda2fc#L0R112

@croaky

croaky Mar 15, 2012

Contributor

Is there a way to use Ruby debug without including it in the repo? I've been on a 'try not to include anything machine- or developer-specific' kick lately.

copycopter/style-guide@1bda2fc#L0R112

@@ -1,7 +1,11 @@
class Api::V2::DraftBlurbsController < Api::V2::BaseController
def index
if stale? :etag => current_project.etag
- render :json => current_project.draft_json
+ if params[:hierarchy]

This comment has been minimized.

Show comment Hide comment
@kareemk

kareemk Mar 15, 2012

Owner

shouldn't the be params[:format] == "hierarchy", isn't that clearer and more flexible

@kareemk

kareemk Mar 15, 2012

Owner

shouldn't the be params[:format] == "hierarchy", isn't that clearer and more flexible

This comment has been minimized.

Show comment Hide comment
@theycallmeswift

theycallmeswift Mar 15, 2012

Member

+1

Gemfile
@@ -24,10 +24,11 @@ end
group :test do
gem 'bourne', '1.1.1'
gem 'capybara-webkit', '0.3.0'
- gem 'cucumber-rails', '0.4.1'
+ gem 'cucumber-rails'

This comment has been minimized.

Show comment Hide comment
@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

add the version back, was seeing an error originally

@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

add the version back, was seeing an error originally

- :draft_content => 'draft two', :published_content => 'published two'
- Factory :localization, :blurb => two, :locale => fr,
- :draft_content => 'ébauche', :published_content => 'publié'
+ Factory :localization, :blurb => one,

This comment has been minimized.

Show comment Hide comment
@kareemk

kareemk Mar 15, 2012

Owner

alignment

@kareemk

kareemk Mar 15, 2012

Owner

alignment

@kareemk

This comment has been minimized.

Show comment Hide comment
@kareemk

kareemk Mar 15, 2012

Owner

Please detail the reason for this change in the pull request. Otherwise LGTM.

Owner

kareemk commented Mar 15, 2012

Please detail the reason for this change in the pull request. Otherwise LGTM.

@@ -1,7 +1,11 @@
class Api::V2::DraftBlurbsController < Api::V2::BaseController
def index
if stale? :etag => current_project.etag
- render :json => current_project.draft_json
+ if params[:hierarchy]
+ render :json => current_project.draft_json(:hierarchy => true)

This comment has been minimized.

Show comment Hide comment
@theycallmeswift

theycallmeswift Mar 15, 2012

Member

:format => :hierarchy

@theycallmeswift

theycallmeswift Mar 15, 2012

Member

:format => :hierarchy

@@ -1,7 +1,11 @@
class Api::V2::PublishedBlurbsController < Api::V2::BaseController
def index
if stale? :etag => current_project.etag
- render :json => current_project.published_json
+ if params[:hierarchy]

This comment has been minimized.

Show comment Hide comment
@theycallmeswift

theycallmeswift Mar 15, 2012

Member

same as above

@theycallmeswift

theycallmeswift Mar 15, 2012

Member

same as above

+ result.deep_merge!({ locale_key => create_hierarchichal_hash_from_array(keys + [content]) })
+ end
+
+ { :data => data, :hierarchichal_data => hierarchichal_data }

This comment has been minimized.

Show comment Hide comment
@theycallmeswift

theycallmeswift Mar 15, 2012

Member

why do you have to send back both? shouldn't this only come back as one or the other?

@theycallmeswift

theycallmeswift Mar 15, 2012

Member

why do you have to send back both? shouldn't this only come back as one or the other?

This comment has been minimized.

Show comment Hide comment
@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

Swift,

The only method that calls this is the Project#generate_json which is in turn used by updated_caches, so if you follow that flow you will see that it takes the data that is returned from this method and stores it in the cache. When we cache we want to cache both versions. We do not need to split out that path.

@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

Swift,

The only method that calls this is the Project#generate_json which is in turn used by updated_caches, so if you follow that flow you will see that it takes the data that is returned from this method and stores it in the cache. When we cache we want to cache both versions. We do not need to split out that path.

@kareemk

This comment has been minimized.

Show comment Hide comment
@kareemk

kareemk Mar 15, 2012

Owner

Need to update docs

Owner

kareemk commented Mar 15, 2012

Need to update docs

+ end
+
+ def down
+ remove_column :text_cahces, :hierarchichal_data

This comment has been minimized.

Show comment Hide comment
@kareemk

kareemk Mar 15, 2012

Owner

typo

This comment has been minimized.

Show comment Hide comment
@croaky

croaky Mar 15, 2012

Contributor

Stylistically, I also like to see add_column when there is only one column changing and change_table do..end for multiple columns. Same style guideline as { ... } for single-line blocks and do...end for multi-line blocks.

@croaky

croaky Mar 15, 2012

Contributor

Stylistically, I also like to see add_column when there is only one column changing and change_table do..end for multiple columns. Same style guideline as { ... } for single-line blocks and do...end for multi-line blocks.

@theycallmeswift

This comment has been minimized.

Show comment Hide comment
@theycallmeswift

theycallmeswift Mar 15, 2012

Member

lgtm

Member

theycallmeswift commented Mar 15, 2012

lgtm

@cmavromoustakos

This comment has been minimized.

Show comment Hide comment
@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

If everyone is ok with this can I get at least one more person to give it a lgtm so I can merge please.

Contributor

cmavromoustakos commented Mar 15, 2012

If everyone is ok with this can I get at least one more person to give it a lgtm so I can merge please.

@croaky

This comment has been minimized.

Show comment Hide comment
@croaky

croaky Mar 15, 2012

Contributor

lgtm

Contributor

croaky commented Mar 15, 2012

lgtm

@cmavromoustakos

This comment has been minimized.

Show comment Hide comment
@cmavromoustakos

cmavromoustakos Mar 15, 2012

Contributor

Deployed with hash commit 8a753c0

Contributor

cmavromoustakos commented Mar 15, 2012

Deployed with hash commit 8a753c0

cmavromoustakos added a commit that referenced this pull request Mar 16, 2012

[#27] Download translations with a hierarchy
The reason for this change is that most Javascript I18n plugins expect
the data in the same hierarchy that it is stored on as the locale yaml
files.

Currently, we have to come up with a conversion to store and retrieve
our keys differently to match the key to the translation. With this
change we allow the flexibility of allowing the end user (CopyCopter
client or some other javascript client) to pass a optional argument
formart with a type of hierarchy to get the data in the format of:

  {
    "en": {
      "test": {
        "one": "expected one",
        "two": "expected two"
      }
    }
  }

croaky added a commit that referenced this pull request Mar 16, 2012

[#27] Use host in install instructions
* Determine it programmatically instead of statically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment