Skip to content

enhancement: Detect polymorphic associations in generator #3645

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

Merged
merged 36 commits into from
Feb 20, 2025

Conversation

Nevelito
Copy link
Contributor

@Nevelito Nevelito commented Feb 6, 2025

Description

Changes:

  • implement detect_polymorphic_associations to resource_ganerator
  • add specs

Fixes # (issue)
#3536

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@Nevelito Nevelito changed the title Chceck potential polymorphic association WIP Detect polymorphic associations in generator Feb 6, 2025
def fields_from_model_tags
tags.each do |name, _|
fields[(remove_last_word_from name).pluralize] = {field: "tags"}
def detect_polymorphic_associations

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method detect_polymorphic_associations has a Cognitive Complexity of 17 (exceeds 5 allowed). Consider refactoring.

Copy link

qlty-cloud-legacy bot commented Feb 6, 2025

Code Climate has analyzed commit d7adb9a and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @Nevelito!

I have a few questions:

@Nevelito Nevelito requested a review from Paul-Bob February 7, 2025 13:52
@Nevelito Nevelito changed the title WIP Detect polymorphic associations in generator enhancement: Detect polymorphic associations in generator Feb 7, 2025
@Nevelito Nevelito requested a review from Paul-Bob February 11, 2025 09:08
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is looking good @Nevelito

Let's just change the message when types are empty and add the types on the test, otherwise is good to merge.

Thank you for this contribution!

@Nevelito
Copy link
Contributor Author

Nevelito commented Feb 11, 2025

I think reduce 3 lines in resource_generator is impossible, we can reduce one here and move first " if " to first line:

fields[name] =
              if association.polymorphic?
                field_with_polymorphic_association(association)
              elsif association.is_a?(ActiveRecord::Reflection::ThroughReflection)
                field_from_through_association(association)
              else
                ::Avo::Mappings::ASSOCIATIONS_MAPPING[association.class]
              end

Of course if we want to keep the code readable

@Paul-Bob
Copy link
Contributor

@Nevelito, I noticed that the generated types aren't constants.

They should be formatted like this: types: [Team, Project, Post, Fish]

Alternatively, types: [::Team, ::Project, ::Post, ::Fish] is also valid.

However, my last suggestion mistakenly converted types: [:Team, :Project, :Post, :Fish] into types: ["Team", "Project", "Post", "Fish"], both of which are incorrect.

@Nevelito
Copy link
Contributor Author

I tried to use constantize witch should fix this issue but it returns types like this

types: [Team(id: integer, name: string, description: text, created_at: datetime, updated_at: datetime, url: string, color: string), Project(id: inte ...

@Paul-Bob
Copy link
Contributor

I tried to use constantize witch should fix this issue but it returns types like this

types: [Team(id: integer, name: string, description: text, created_at: datetime, updated_at: datetime, url: string, color: string), Project(id: inte ...

I know, I don't know a easy way to achieve this, could you do some research on this please?

@Nevelito
Copy link
Contributor Author

I created alias with new inspect function and keep original inspect in function original_inspect, now it works as it should but let me know what you think

@Paul-Bob
Copy link
Contributor

I created alias with new inspect function and keep original inspect in function original_inspect, now it works as it should but let me know what you think

@Nevelito you did that change on the spec/dummy/app/models/application_record.rb which is inside Avo's dummy application. What will happen on user's applications that will not have this monkey patch? Most provably will not work as expected and the types will be rendered as: types: [Team(id: integer, name: string, description: text, created_at: datetime, updated_at: datetime, url: string, color: string), Project(id: inte ... which is not ideal. We want this way of rendering types: types: [Team, Project, Post, Fish] on all the applications

Suggesting this change to all the users is not ideal and monkey patching the ActiveRecord::Base it's also not reasonable.

Let's try to achieve this output: types: [Team, Project, Post, Fish] without monkey-patches if possible

@Nevelito
Copy link
Contributor Author

I am not sure if it is possible, I tried to find something but nothing works, only changing inspect gave us wanted result but I see that we can not make like that because of this is in dummy app. I not sure what can we do now.

@Paul-Bob Paul-Bob merged commit 040bb9e into avo-hq:main Feb 20, 2025
19 of 20 checks passed
@Paul-Bob
Copy link
Contributor

Thanks you for this contribution @Nevelito !

@Paul-Bob Paul-Bob added the Enhancement Not necessarily a feature, but something has improved label Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not necessarily a feature, but something has improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants