-
Notifications
You must be signed in to change notification settings - Fork 140
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
Default ID should be 'id', not based on name #26
Comments
ah, the joys of controller abstractions :D (this kind of thing is why I hate rails). I dont disagree with the id thing, though if/when others are involved switching to forum or forum_id would be annoying |
@aseemk, can you be more specific about where you ran into the problem? I agree that the 'current resource' ID should always be 'id'. But, if you are nested, then your parent 'id's need to be called something else. I generally expect that, for example, '/forums/12/threads/15' in the thread controller you will have id === 15 and forum_id === 12. Is this what you mean? @tj, I don't see how you get away from a controller of some sort for a resource. You need something to bind the incoming request with the underlying data models while applying authentication/authorization controls and rendering the proper result. You don't have to call it a controller, but somewhere you want that code. |
@pacovell sure, to a certain degree yeah, js just does not have an "elegant" way to express this stuff. when you get away from object literals things get so ugly you might as well just use app.get() directly and benefit from clarity |
I prefer the current way, besides if all resource params will be named as I think it is more clear as it is now. |
Just spent a bunch of time debugging what turned out to be caused by us reusing some code that said
req.params.id
for a non-top-level resource.IMHO, having the API change based on what resource you're in doesn't exactly meet the principle of least surprise. =D
Also, FWIW, from a usage perspective,
id
is a much better variable name because it's actually a string, whereas e.g.forum
or w/e isn't accurate, because it's not aForum
object (or whatever my model-layer objects are).So here's my ask for the default ID to be consistent and simple. You could still pass in a custom ID as an option, etc.
The text was updated successfully, but these errors were encountered: