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 api old element actions and path helpers #4557

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

AntonKhorev
Copy link
Collaborator

After element version paths were added to the website, node_version_path etc path helpers became confusing because they point to the api. Some of the api path helpers have the api_ prefix, others don't. Ideally all of them should have it. Let's start with *_version_path which, if named similarly to web path helpers, should be api_old_node_path. Also controller actions, again if named similarly to web actions, should be show instead of version in this case. OldNode etc instances are already versions, it's strange to ask for a version of a version.

@tomhughes
Copy link
Member

I think historically we've only added the api prefix where we needed to disambiguate, something which I ran into last weekend when I needed to do exactly that for the changeset unsubscribe action.

I think it probably does make sense to do it consistently though to avoid any future issues with conflicts though.

The only thing I'm not sure about here is the user of resources and treating the version as the key for the resource - it's not exactly the normal rails model but then having a composite key made up of id+version isn't either... I'd like to here what @gravitystorm thinks before merging this one though.

@AntonKhorev AntonKhorev force-pushed the rename-api-old-version branch from 9206fd8 to ad96da4 Compare March 4, 2024 08:30
@AntonKhorev
Copy link
Collaborator Author

I removed resources because it isn't the main point here. I only put it because earlier I was told to do so. This is just three lines which I can add later if required.

@tomhughes
Copy link
Member

Look I didn't ask you to remove that! I said I wasn't sure about it and asked @gravitystorm, as somebody with a better grasp of "canonical rails" what he thought so that we could make a decision.

Now you tell me that the idea in fact came from @gravitystorm which is great, but rather than just say that you reverse the change?

Not every comment I make needs to be acted on - sometimes I will be wrong! When I ask a question it is exactly that, a question, not an instruction to make a change. Please feel free to push back if you have an alternate view when I ask a question.

@AntonKhorev
Copy link
Collaborator Author

This change is 3 lines in one file. Everything else in this PR is across 17 files. If you are not sure about the change why wouldn't you want it in a separate PR?

@gravitystorm
Copy link
Collaborator

I think it probably does make sense to do it consistently though to avoid any future issues with conflicts though.

I agree. I think there's probably some combination of scope/namespace/module/path/as declaration that would achieve this for all the api resources, but I always find it a bit tricky to know which one to use without testing every possibility. I spent a long time exploring these while trying to implement multiple-api-versions a few years ago, and I still haven't quite got to grips with them.

@gravitystorm
Copy link
Collaborator

Let's start with *_version_path which, if named similarly to web path helpers, should be api_old_node_path. Also controller actions, again if named similarly to web actions, should be show instead of version in this case. OldNode etc instances are already versions, it's strange to ask for a version of a version.

Looks good to me, thanks!

@gravitystorm gravitystorm merged commit ac01ada into openstreetmap:master Mar 13, 2024
20 checks passed
@AntonKhorev AntonKhorev deleted the rename-api-old-version branch March 13, 2024 15:59
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.

3 participants