Skip to content

refactor: via_resource_class param #3383

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

Closed

Conversation

thiagoyoussef
Copy link
Contributor

Description

Fixes #2100

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

Screenshots & recording

CleanShot 2024-11-03 at 11 14 59@2x

Copy link

codeclimate bot commented Nov 3, 2024

Code Climate has analyzed commit f1f13b4 and detected 0 issues on this pull request.

View more on Code Climate.

@thiagoyoussef
Copy link
Contributor Author

Considered using:

  • avo/resources/user -> avo%2Fresources%2Fuser
  • avo__resources__user

I found the simpler solution is to demodulize the resource class to User. This approach allows us to avoid encoding and at the same time is not needed to format the string back to the class inside Avo.resource_manager.get_resource method:

def get_resource(resource)
  # Already do the job
  resource = "Avo::Resources::#{resource}" unless resource.to_s.starts_with?("Avo::Resources::")
  ...
end

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.

Thank you for this contribution @thiagoyoussef

The idea behind refactoring via_resource_class is to make it cleaner on the params, while keeping the namespace structure.

There should be a clear difference on params between Avo::Resources::Link and Avo::Resource::Course::Link.

Unfortunately, this approach loses all namespace context.

This issue needs some research and some discussion around the implementation before actually coding.

There are some issues marked as "Good first issue" if you want to contribute on something where you can jump directly to the development phase.

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Nov 4, 2024

Closing this one until we research a proper approach. Thank you again @thiagoyoussef for all the effort on this one

@Paul-Bob Paul-Bob closed this Nov 4, 2024
@thiagoyoussef
Copy link
Contributor Author

100%, I didn't think about the context when there is Avo::Resources::Link and Avo::Resource::Course::Link.
@Paul-Bob thanks for the feedback!

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.

Refactor the via_resource param
2 participants