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

Convert dash (and more) to underscore in URL helper names #281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

svoop
Copy link

@svoop svoop commented Feb 14, 2025

Given a scope (or by extension: a slice) which contains dash -, plus +, tilde ~ or dot . in their URL fragment:

# config/routes.rb

module Dashy
  class Routes < Hanami::Routes
    scope "my-scope" do
      get "foo", to: "action.foo", as: :foo
    end
  end
end

The dashes etc. are converted to underscores for the corresponding URL helper name:

% hanami routes

GET   /my-slice/foo   action.foo   as :my_slice_foo
        ^^^                             ^^^

Closes #280

Copy link
Member

@cllns cllns left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for doing this. A few small comments but this looks great :)

@@ -693,6 +693,10 @@ def env_for(env, params = {}, options = {})
# @api private
PREFIXED_NAME_SEPARATOR = "_"

# @since 2.3.0
# @api private
NAME_UNDERSCORE_REGEXP = /[\-+~.]/.freeze
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the leading \ does anything, since it just acts as an escape, and - doesn't need to be escaped at the beginning of a regexp. It only acts as a regexp separator when it's between two characters. I'm also fine with keeping it though, just to be explicit

Copy link
Author

Choose a reason for hiding this comment

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

Well spottet, @cllns !

Actually, I quote - inside square brackets always and on purpose: Not everybody is experienced enough with Regexp to know and if you add another acceptable character to the front of the list, the unquoted - silently flips from being a literal to a range operator. And if this someone is out of luck, the resulting range includes the literal - and therefore the tests might not even fail. This quite likely wouldn't be a huge problem here, but it could introduce a difficult to spot bug or even open a security gap elsewhere. Thus, better safe than sorry.

But feel free to insist or change it if you prefer dropping the quote.

@@ -3,13 +3,18 @@
RSpec.describe Hanami::Router do
let(:router) do
e = endpoint
# rubocop:disable Layout/ExtraSpacing
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this and just comply with the rule instead? It's not as clean looking but it's still fine, especially since this is just a spec

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing, removed it.

@@ -878,6 +882,12 @@ def prefixed_name(name)
@name_prefix.relative_join(name, PREFIXED_NAME_SEPARATOR).to_sym
end

# @since 2.3.0
# @api private
def underscored_name(name)
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like prefixed_name is used anywhere else, and it's defined as a private method + private API, so can we just push the underscoring into that method? It'll save some coercion back-and-forth between symbol and string, but more importantly, it keeps thing simple and doesn't overly abstract helper methods.

Copy link
Author

@svoop svoop Feb 15, 2025

Choose a reason for hiding this comment

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

Absolutely, it now reads:

    def prefixed_underscored_name(name)
      @name_prefix
        .relative_join(name, PREFIXED_NAME_SEPARATOR)
        .gsub(UNDERSCORED_NAME_REGEXP, "_")
        .to_sym
    end

Hope, the multi-line notation with leading dots is okay, opinions and tastes differ on this one.

Rubocop appears to be okay with it, but it barks at the constants being declared following private:

Useless private access modifier for constant scope.

How do you deal with such situations?

  • Move private or exclude the cop rule?
  • Mop this up as part of an existing PR like this one or create a fresh PR for it?

@svoop svoop force-pushed the 280_underscored_url_helpers branch from 65112a1 to 023a8e9 Compare February 15, 2025 00:29
Also convert plus, tilde and dot which are equally allowed in URLs.
@svoop svoop force-pushed the 280_underscored_url_helpers branch from 023a8e9 to 43fd33c Compare February 15, 2025 08:52
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.

Convert dashes in scope/slice URL fragments to underscores in URL helpers
2 participants