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

Not able to use transformer when it is in included view #225

Closed
bhooshiek-narendiran opened this issue Jul 13, 2020 · 12 comments · Fixed by #372
Closed

Not able to use transformer when it is in included view #225

bhooshiek-narendiran opened this issue Jul 13, 2020 · 12 comments · Fixed by #372
Labels
bug An issue with the system no-issue-activity pinned

Comments

@bhooshiek-narendiran
Copy link
Contributor

bhooshiek-narendiran commented Jul 13, 2020

view :skeleton do
    include_view :summary
    excludes :actors, :ticket_id
  end

  view :summary do
    field :actors_response_info, name: :actors
    transform FieldTransformer
  end

Here when I directly use the view: summary then the FieldTransformer is working. But I can't able to use the same transformer when it is included in another view. Like if view: skeleton is called, the FieldTransformer is not working. This is because we are not taking the included views transformer while rendering the output.
Can I raise a pr for this one?

@bhooshiek-narendiran bhooshiek-narendiran changed the title Not able to use included views transformer Not able to use transformer when it in included view Jul 13, 2020
@bhooshiek-narendiran bhooshiek-narendiran changed the title Not able to use transformer when it in included view Not able to use transformer when it is in included view Jul 13, 2020
@adamwells
Copy link
Contributor

@bhooshiek-narendiran Good find, thank you - definitely feel free to open a PR to solve this.

@bhooshiek-narendiran
Copy link
Contributor Author

Thanks, @adamwells Will do.

@aarona
Copy link

aarona commented Oct 16, 2020

Could really use this. Would be great if someone could do a code review and accept this PR. @bhooshiek-narendiran, thanks for fixing this btw.

@cooperka
Copy link

Thanks for the PR @bhooshiek-narendiran!

Separate but related issue: views should inherit transformers from the base class but they do not. This is demonstrated by a new PR #247. It is unfortunately not fixed by your change.

I wonder if we could make a generic fix for both issues... I don't currently have time to dig deeper but wanted to at least share these preliminary findings here (a workaround for me is to simply re-include the transformer in every single view, so that's what I'm doing until I have time to help with this proper fix).

@bhooshiek-narendiran
Copy link
Contributor Author

HI @cooperka! Can you elaborate me the above issue with an example? So I'll try to fix that one in this PR.

@cooperka
Copy link

Yes please see the linked PR #247 with a working unit test (it's failing, in order to demonstrate the issue) 🙂 Let me know if you have questions related to it, thanks for your help!

@bhooshiek-narendiran
Copy link
Contributor Author

Hi, @cooperka One doubt whether the view transformer should override the class transformers(parent and child) or both should execute (view transformers + class transformers)?

@cooperka
Copy link

Hmm, if both execute, is there a way to STOP the parent from executing through some syntax? If there's no way to stop it, I think it must override entirely. But I will leave that up to the library author during PR review.

@bhooshiek-narendiran
Copy link
Contributor Author

currently, we have options to differentiate default ones(parent & child class transformers) and view transformers. So I thought, We can use view transformers if it is configured or else (parent or child class transformers) will execute. If we need to block parent class transformers alone then we need some parameter to differentiate that one.

@aarona
Copy link

aarona commented Nov 20, 2020

Following this thread. Just wanted to chime in real quick. This might be over engineering but maybe a solution similar to how rack middleware Is implemented?

@cooperka
Copy link

My advice is "keep it simple", whatever it does can be documented in the readme. If using BOTH transformers and adding new syntax for blocking the parent is easier, that's fine with me, but my assumption is that overriding would be easier (because that seems to be how it currently works, no change needed).

The only current issue for me is when a transformer is NOT defined in a view, it SHOULD inherit the base transformer, and currently it does not.

@lessthanjacob lessthanjacob added the bug An issue with the system label Jul 21, 2023
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the system no-issue-activity pinned
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants