Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add :namespace option to decorate #899

Closed
wants to merge 2 commits into from
Closed

Add :namespace option to decorate #899

wants to merge 2 commits into from

Conversation

n-rodriguez
Copy link
Contributor

@n-rodriguez n-rodriguez commented Jan 25, 2021

This PR is a rebase on master branch of @ivan-denysov work. It also contains some fixes to finalize the implementation. See #835 (comment)

ping @codebycliff

it closes

@n-rodriguez n-rodriguez changed the title Wip/namespace Add :namespace option to decorate Jan 25, 2021
@dan-jensen
Copy link

@n-rodriguez huge respect for the work you've put into this, it looks like you've done a great job. I was just struggling with decorating a namespaced model for a while and came across this PR.

I wanted to comment because ended up solving my problem a very simple way: by namespacing the decorator the same way as the model. In my case, app/decorators/help/article_decorator.rb defines class Help::ArticleDecorator < Draper::Decorator. Given that this works, I wonder if it's the best solution because 1) it already exists, 2) it relies on convention over configuration, and 3) it plays nice with Rails autoloaders (both classic and zeitwerk both complain if the file namespace doesn't match the class namespace - "expected {FILE} to define constant {CLASS}"). But I realize your code may have benefits I'm not aware of. What do you think?

I just pushed a PR with a namespace example. (I don't believe it conflicts with this PR in any way.) Would love to hear your comments on it because you know so much about this area 🙂

@n-rodriguez
Copy link
Contributor Author

n-rodriguez commented Jan 28, 2021

by namespacing the decorator the same way as the model.
But I realize your code may have benefits I'm not aware of. What do you think?

It is indeed a solution. But you may not want to do that (namespacing the decorator the same way as the model) but instead use more "conceptual" namespaces.

For instance, my Rails app is splited into 3 different websites (1 Rails app with 2 engines) sharing the same database. So there are 3 decorators namespaces : one for each websites and those namespaces are not bound to a specific model.

It looks like :

module BackOffice
  # resides in Rails app : rails_root/app/decorators/back_office/application_decorator.rb
  class ApplicationDecorator < Draper::Decorator
  end

  class UserDecorator < ApplicationDecorator
  end
end

module FrenchSite
  # resides in engine : engine/french_site/app/decorators/french_site/application_decorator.rb
  class ApplicationDecorator < Draper::Decorator
  end

  class FooDecorator < ApplicationDecorator
  end
end

module SpanishSite
  # resides in engine : engine/spanish_site/app/decorators/spanish_site/application_decorator.rb
  class ApplicationDecorator < Draper::Decorator
  end

  class BarDecorator < ApplicationDecorator
  end
end

You can do that only with this PR.

@n-rodriguez
Copy link
Contributor Author

Ping @codebycliff

@n-rodriguez
Copy link
Contributor Author

Hi there! Any news?

@n-rodriguez
Copy link
Contributor Author

Ping @codebycliff. It would be nice to have some news...

I hope I've not worked for nothing....

@n-rodriguez
Copy link
Contributor Author

ping @codebycliff

@n-rodriguez
Copy link
Contributor Author

ping @codebycliff

@n-rodriguez
Copy link
Contributor Author

is it possible to have some fucking news?

@codebycliff
Copy link
Collaborator

@n-rodriguez While I understand your frustration and apologize for the delay, I don't appreciate the language and don't really think anyone in the industry would either. We are all volunteers here – where we are in fact working for nothing. The reason for my delay is two fold:

The first being I simply don't have the time to maintain this gem anymore. My work and personal responsibilities have changed significantly since I took on the maintenance of this gem and I don't have the same amount of free time that I once did.

The second reason is the nature of this change. This pull request is adding more functionality around inference, which is the largest headache of this gem. When I originally took over maintenance of this gem, the original author (@jcasimir) really wanted me to remove the inference entirely per this issue. While I never was able to lead up that effort, I'm hesitant to introduce more functionality around the #decorate method. I alluded to this hesitancy in the original issue that was opened. The direction the original author wanted to go was adding functionality like this through a contrib package that adds the functionality. I'd recommend this approach if you need to get this functionality out into the wild quickly. Otherwise, the little time I have to dedicate to this gem will be focused on bug fixes and releasing new versions.

If you (or anyone else) would like to volunteer to helping maintain the repository going forward, that is something we can discuss. I just don't want to merge new features and not be around to support them. Let me know your thoughts. Again, I apologize for the delay.

@tovodeverett
Copy link
Contributor

From the sidelines, a few observations:

  • I totally understand the frustration @n-rodriguez expressed (3.5 months is a long time), and while I agree with @codebycliff that the language used in the comment was inappropriate, I can also understand where it comes from.
  • I totally understand how life can take over and make it hard to find time for even short efforts. I don't know what's going on in @codebycliff's life, but things changed dramatically for me when I started having kids. If I can't work on something during paid time, it's a struggle to decide between scratching a personal itch and spending time with my kids so that they can do things we love to do (skiing, mountain biking, etc.).
  • Hopefully @codebycliff's measured response will lead to a de-escalation and attract some new blood into helming the project so that @codebycliff can share the reins with someone who has the time to shepherd the project. That would be an awesome outcome for those of us who rely upon draper, but definitely don't have the time or skills to manage it.

@n-rodriguez
Copy link
Contributor Author

n-rodriguez commented Aug 9, 2021

blablabla... blablabla... blablabla...

@n-rodriguez n-rodriguez closed this Aug 9, 2021
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.

None yet

4 participants