search

jipiboily / spree_multi_lingual

Spree multi-lingual is a plugin to make multi locale store with Spree possible
BSD 3-Clause "New" or "Revised" License
49 stars 56 forks source link

merge 1-3-stable and 1.2 #38

Closed sbounmy closed 11 years ago

sbounmy commented 11 years ago

@bricesanchez @jipiboily @stevenjenkins I think we need to sync 1.2 and 1-3-stable branches. And then I will be able to start merging pull requests and maintain a master branch.

@bricesanchez I've started a looking at your branch 1.2 (if I remember correctly) : What are the models 'ContentChunk', 'Video' is this a custom branch ?

bricesanchez commented 11 years ago

@sbounmy, You're right ! The models are for spree_content_chunk and spree_videos gems. I use thoses gems in my spree project and i think it's a good idea to include all multi lingual work on spree in this extension (spree_multi_lingual).

jipiboily commented 11 years ago

For what it's worth, I actually disagree. I think SpreeMultiLingual should only include the work necessary for the core. Multi lingual for extensions should be handled in their own extensions, or something like that. Possibly something like spree_multi_lingual_my_extension. Or it could be handled in my_extension itself. Not sure what is the best.

In it's current form, this will lead to errors due to the original class not being in the project:

irb(main):001:0> ContentChunk.class_eval do; end
NameError: uninitialized constant ContentChunk
    from (irb):1
    from /usr/local/opt/rbenv/versions/1.9.3-p392/bin/irb:12:in `<main>'

Yes, you could have conditions and only apply that if the class exists...but it it's starts to be a bit hacky and sounds like a code smell to me. My 2 cents! ;)

That said, I don't work with Spree anymore, so it's up to you guys to decide.

bricesanchez commented 11 years ago

@jipiboily You're right. If it's break when there is no original class, we should create a spree_multi_lingual_my_extension instead of including it in SpreeMultiLingual.

sbounmy commented 11 years ago

@bricesanchez I see, as @jipiboily said your branch cannot be used unless we install all your dependencies spree_videos, etc.

You can checkout how I added spree multilingual extension to spree_essential_cms https://github.com/sbounmy/spree_essential_cms/commit/d2932dd97dd67614adb96908c66190538f9fea54

I am thinking to merge 1-3-stable into master and start merging PR / fix issue.

sbounmy commented 11 years ago

Guys,

I've done some refactoring and fixed specs, its now green on travis. I've merged 1-3-stable in master. if anyone want to support spree master (2.0 alpha) I would be glad to merge his PR in master :)

From now I will start merging p/r and closing issue

jipiboily commented 11 years ago

:heart: