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

Fixed argument prefix warnings #3594

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

iainbeeston
Copy link
Contributor

Description

If I run an app using avo with warnings enabled I get a LOT of warnings like this:

avo-3.16.1/app/components/avo/fields/files_field/edit_component.html.erb:1: warning: `**' interpreted as argument prefix

This is happening because a function is being called with ** as the first argument but there are no brackets around the arguments. Adding brackets fixes the warning but it does not change the meaning, so I've done that for all field components.

Fixes # (issue)

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

Manual review steps

  1. Everything should work as before (this shouldn't change any behavior)

Manual reviewer: please leave a comment with output from the test if that's the case.

If I run an app using avo with warnings enabled I get a LOT of warnings like this:

    avo-3.16.1/app/components/avo/fields/files_field/edit_component.html.erb:1: warning: `**' interpreted as argument prefix

This is happening because a function is being called with `**` as the first argument but there are no brackets around the arguments. Adding brackets fixes the warning but it does not change the meaning, so I've done that for all field components.
Copy link

codeclimate bot commented Jan 20, 2025

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

View more on Code Climate.

@Paul-Bob
Copy link
Contributor

Hi @iainbeeston thanks for this contribution.

If I run an app using avo with warnings enabled I get a LOT of warnings like this:

What warnings are you enabling to see this logs?

@iainbeeston
Copy link
Contributor Author

@Paul-Bob Sorry I'm talking about warnings coming from ruby itself. You can enable that by either:

  • running ruby with the -w flag
  • running rspec with the --warnings flag
  • Setting $VERBOSE=true (in a rails app this can be done in your puma.rb config file or anywhere called during startup)

In rspec the errors appear in the console, in rails they appear in the server logs when you navigate to any avo resource page.

I'm using ruby 3.3.2 and looking at the source code for ruby it looks like this warning was added in ruby 3.3

@Paul-Bob
Copy link
Contributor

Thanks for sharing those @iainbeeston

Is there any rule that we can add to our linter to avoid this kind of warning in the future?

@iainbeeston
Copy link
Contributor Author

That's a really good question... ideally there would be a rubocop cop about this but I've looked and I can't see one (except always requiring parentheses). Even if there was a rubocop cop you'd also need to be using ere-lint because these warnings are coming from view files (I'm not sure if you are using ere-lint or not?)

@Paul-Bob
Copy link
Contributor

I'm not sure if you are using ere-lint or not?

Do you mean erb-lint? If so, we're not using any right now.

Even if there was a rubocop cop you'd also need to be using ere-lint because these warnings are coming from view files

Makes sense, thank you!

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 this contribution @iainbeeston!

@Paul-Bob Paul-Bob merged commit 20a3646 into avo-hq:main Jan 21, 2025
21 checks passed
@adrianthedev
Copy link
Collaborator

adrianthedev commented Jan 21, 2025

Has anyone try Dangerfile before?

Maybe we can set up an easy quick alert with that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants