-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 :)
lib/hanami/router.rb
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
spec/unit/hanami/router/url_spec.rb
Outdated
@@ -3,13 +3,18 @@ | |||
RSpec.describe Hanami::Router do | |||
let(:router) do | |||
e = endpoint | |||
# rubocop:disable Layout/ExtraSpacing |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, removed it.
lib/hanami/router.rb
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
65112a1
to
023a8e9
Compare
Also convert plus, tilde and dot which are equally allowed in URLs.
023a8e9
to
43fd33c
Compare
Given a scope (or by extension: a slice) which contains dash
-
, plus+
, tilde~
or dot.
in their URL fragment:The dashes etc. are converted to underscores for the corresponding URL helper name:
Closes #280