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

Enhancement: improve message when missing array resource for array fields #3730

Conversation

zhephyn
Copy link
Contributor

@zhephyn zhephyn commented Mar 10, 2025

Description

Enhances the thrown missing resource error such that when the field is of the type array, the error is thrown with an array flag "--flag"

Fixes # (#3713)

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

Copy link

codeclimate bot commented Mar 10, 2025

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

View more on Code Climate.

@Paul-Bob
Copy link
Contributor

Thanks for submitting this contribution @zhephyn

Please ask me to review when you consider it ready to review.

image

@zhephyn
Copy link
Contributor Author

zhephyn commented Mar 10, 2025

Hey @Paul-Bob. I think I've successfully implemented the enhancement as per the scope you suggested. The current failing tests indicated in the CI output seem to be beyond the scope of what we were trying to do with this PR. For example, the one below

`2) Localization spec force_locale translates the resource name on the create button
Failure/Error: <%= a_link url_for(params.permit(:view_type, :turbo_frame, :view, :resource_name, :id, :related_name, :sort_by, :sort_direction).merge(view_type: type)).to_s,

 ActionView::Template::Error:`

Should i look into these as well?
In case of anything else error related which may apply to the current enhancement as per this PR, kindly let me know and I'll look into it.
Also, in case you get some time, kindly tell me what you think about the code after reviewing. Thanks

@Paul-Bob
Copy link
Contributor

Hi @zhephyn

The current failing tests indicated in the CI output seem to be beyond the scope of what we were trying to do

Failing tests on a PR are never beyond its scope if those same tests are passing on main, which they are. This suggests that changes introduced in this PR are causing the failures. Let's ensure the tests pass before proceeding.

Also, in case you get some time, kindly tell me what you think about the code after reviewing. Thanks

I took a quick look and noticed several changes unrelated to the issue, for example, the changes on app/controllers/avo/actions_controller.rb. Additionally, some modifications appear to revert recent commits. Let's clean up the PR so it includes only the relevant changes. Once it's refined and tests are passing, feel free to request a review. Let me know if you encounter any blockers.

@zhephyn
Copy link
Contributor Author

zhephyn commented Mar 10, 2025

@Paul-Bob . Regarding how to proceed, the idea i had in mind was deleting this current branch and therefore this PR, creating a new branch locally, implementing the enhancement as per the scope, pushing it to Github and then opening a new PR. This new branch will focus solely on the scope as expected. Is this a good approach or is their perhaps a better one i could use to clean up the branch?
Otherwise, I'll have the cleaned up branch version and PR by you latest tomorrow evening.

@Paul-Bob
Copy link
Contributor

Yes, @zhephyn, a new PR sounds great.

Before starting a new branch, please ensure that main is up to date.

@zhephyn zhephyn closed this Mar 11, 2025
@zhephyn zhephyn deleted the enhancement/improve-message-when-missing-array-resource-for-array-fields branch March 11, 2025 00:42
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.

2 participants