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

do not alias rich text area if it is not defined #709

Conversation

BenMorganMY
Copy link
Contributor

We experienced an error with eager loading in staging for bootstrap_form. This small if statement resolves that issue.

@BenMorganMY BenMorganMY changed the title do not alias rich rext area if it is not defined do not alias rich text area if it is not defined Oct 5, 2023
Copy link
Contributor

@lcreid lcreid 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 PR. I really appreciate the time you're dedicating to make bootstrap_form better. I'm no expert on how Rails loads, but I'm guessing that when the tests run, our code is being run before rich_text_area is defined.

I really think we should get this to work the way you want it, but also have all the tests passing. Feel free to take a swing at it, or just comment back that you want someone else to take a run at it.

@BenMorganMY
Copy link
Contributor Author

@lcreid I've switched from using the check to now just silencing the NameError that gets raised when there's a bad alias. I've opted to disable the lint for it and add docs explaining the decision on why it's there. I see this as the simplest way forward.

Copy link
Contributor

@lcreid lcreid left a comment

Choose a reason for hiding this comment

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

I appreciate the effort you're putting into this. I think your approach is reasonable. I'm going to take a look at the problem as soon as I can and see if there's just something about the way we're loading that should be looked at. I suspect there is. This is not the first time we've had issues around loading the tests.

def bootstrap_alias(field_name)
alias_method "#{field_name}_without_bootstrap".to_sym, field_name
alias_method field_name, "#{field_name}_with_bootstrap".to_sym
rescue NameError
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to do the RuboCop exception specifically on the line.

Suggested change
rescue NameError
rescue NameError # rubocop:disable Lint/SupressedException

.rubocop.yml Outdated
Comment on lines 40 to 42
Lint/SuppressedException:
Enabled: false

Copy link
Contributor

Choose a reason for hiding this comment

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

See below.

Suggested change
Lint/SuppressedException:
Enabled: false

@@ -45,6 +48,7 @@ Metrics/AbcSize:

Metrics/BlockLength:
Exclude:
- "lib/bootstrap_form/inputs/base.rb"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can leave this one in here, since it applies to the whole file anyway. But I should look at the rule and where it came from. I'm a fan of RuboCop, but not of these rules that put arbitrary limits on the size of things.

@BenMorganMY
Copy link
Contributor Author

@lcreid updated the rubocop lint suppression suggestion.

Copy link
Contributor

@lcreid lcreid 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 PR!

@lcreid lcreid merged commit e838d4e into bootstrap-ruby:main Oct 28, 2023
10 of 11 checks passed
@BenMorganMY BenMorganMY deleted the fix-bootstrap-alias-with-actiontext-not-present branch October 30, 2023 16:21
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