-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add a style guide to suggest to pass a string literal as a URL to http method calls #359
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
Conversation
...p method calls Original discussion: rubocop#328 Signed-off-by: moznion <moznion@mail.moznion.net>
I’m in general in favour of this change, but I admit that I’m biased because I’m mostly working on APIs, and the general picture may not be as simple as the limited use case I got used to.
Rails guides say (about the use of path helpers in views):
patient_path(@patient)then the router will generate the path /patients/17. This reduces the brittleness of your view and makes your code easier to understand
A lot of to digest in this brief note:
- Reduces brittleness. Path helpers add an abstraction over a detail, the URL path. It’s a design decision whether those paths should be stable or can be bent at will. For the public API or pages that will be bookmarked, shared or referenced from other resources - it makes sense to keep stable. For internal APIs, APIs for own mobile apps or web - not really.
- Easier to understand. I don’t really take this argument. Pretty sure this can be stretched in a way that the URL will be easier to understand than a path helper.
What is the source of the brittleness?
A mistake causing a change to the URL. Or a deliberate routes refactoring.
It can indeed cause views and redirects to point to the outdated route.
But with tests?
Should we introduce a concept of route stability, and say that this only makes sense as a measure to ensure route stability? And otherwise path helpers are preferrable to reduce brittleness?
What do you think?
I appreciate your explanation.
Easier to understand. I don’t really take this argument. Pretty sure this can be stretched in a way that the URL will be easier to understand than a path helper.
Yeah, I’m on the same page, and the main motivation comes from this.
Anyway, my understanding of the term "brittleness" is related to path-traversal attacks, where malicious "URL-ish" arguments are injected. I completely agree with the approach of using helpers with URL escaping on the given argument to protect against this type of vulnerability.
However, in testing, this shouldn’t be a problem since it’s just a test. Personally, I believe that other issues, like breaking changes to endpoints, should be caught by the tests.
Based on #328 shouldn't this section mention that it applies to tests and not production code?
@dvandersluis Thank you for your suggestion; it sounds great to me. However, what would be the best approach: should I include that in the description, or move it under the "Testing" section?
Putting it under Testing would make sense to me!
Signed-off-by: moznion <moznion@mail.moznion.net>
@dvandersluis Thank you, I moved it: 1c4cf0e
@moznion you might also want to mention tests in the PR title, I was confused the first time reading this.
Original discussion: #328