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

Rename to actionview html sanitizer #171

Closed

Conversation

tongueroo
Copy link

@tongueroo tongueroo commented Oct 20, 2023

Why

This PR renames the rails-html-sanitizer to actionview-html-sanitizer.

Currently, the rails-html-sanitizer is used in actionview and defines an empty Rails namespace. This makes it more challenging to use with frameworks like Jets, because once the Rails constant is defined, then other gems that do this:

some_gem/railtie.rb

require "some_gem/railtie" if defined?(Rails)

won't be able to be used with Jets. Renaming to actionview-html-sanitizer gives other gems a chance to work with Jets, also.

Note: The ideal way to check would for the other gems to use this instead:

require "some_gem/railtie" if defined?(Rails::Railtie)

It's a bummer, gems authors are not always aware of this.

Related

Copy link
Member

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

How do you suggest we avoid breaking existing Rails apps that are referencing constants like Rails::Html::Sanitizer?

I appreciate that you're trying to solve a real problem, but I do not think that renaming this project and renaming the top-level module is an appropriate solution. At the very least, you are presuming that the maintainers of this library are willing to take on a great deal of work to ensure that existing applications aren't broken by this change, and I remain unconvinced that this library needs to own that responsibility.

@@ -24,19 +24,19 @@ Performance:
- '**/test/**/*'

# Prefer assert_not over assert !
Rails/AssertNot:
ActionView/AssertNot:
Copy link
Member

Choose a reason for hiding this comment

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

It is not correct to change this name, these rules are defined in https://docs.rubocop.org/rubocop-rails/cops_rails.html#railsassertnot with the Rails namespace.

Copy link
Author

Choose a reason for hiding this comment

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

Oops. Ok

module HTML
class Sanitizer
VERSION = "1.6.0"
VERSION = "2.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

It's inappropriate to make a version change in a feature PR.

Copy link
Author

Choose a reason for hiding this comment

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

Thank for the note.

@tongueroo
Copy link
Author

All good, I understand. Can workaround for Jets. Closing out. Thanks for the response though!

@tongueroo tongueroo closed this Oct 22, 2023
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