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

Method naming conventions #61

Merged
merged 5 commits into from
Feb 25, 2024
Merged

Method naming conventions #61

merged 5 commits into from
Feb 25, 2024

Conversation

ksol
Copy link
Contributor

@ksol ksol commented Feb 20, 2024

Introduce helper scripts, then rename methods to use consistent names:

  • list for fetching a collection of resources. Example: keys.list
  • find for fetching a singular resource. Example: keys.find(id: 'key')
  • create for creating a resource. Example: keys.create(name: 'key')
  • update for updating a resource. Example: keys.update(id: 'key', name: 'new-key')
  • delete for deleting a resource. Example: keys.delete(id: 'key')

(this will end up in the readme, yep)

Related resources (eg: containers.sizes, addons.categories, repo_link.branches) stay the same for now, but they might be renamed/moved around too later.

Specs are adapted. Some aliases are removed, and there's a specific case for events for which URI templates are not well adapted: if there's an app_id supplied, we need to prefix it with /apps, but adding a static part if a variable is expanded doesn't seem to be possible.

@ksol ksol self-assigned this Feb 20, 2024
@aurelien-reeves-scalingo
Copy link
Contributor

  • update for updating a resource. Example: keys.create(id: 'key', name: 'new-key')

Typo here

Copy link
Contributor

@aurelien-reeves-scalingo aurelien-reeves-scalingo left a comment

Choose a reason for hiding this comment

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

This is a really, really big breaking change. I see a very high risk for users not updating the gem because of this.

What about keeping the old methods (using alias) and deprecating them at first?

@ksol
Copy link
Contributor Author

ksol commented Feb 20, 2024

I see a very high risk for users not updating the gem because of this.

On one hand, it's not more breaking than the changes already in master, and on the other hand there is about 3k downloads for the gem for 2023, which is pretty low.

With a high adoption rate, your suggestion would make a lot of sense, but in the current context, I'd be surprised if besides us, there were more than 10 persones/teams actively relying on the gem

@ksol ksol enabled auto-merge (squash) February 25, 2024 19:44
@ksol ksol merged commit 8fabc2b into master Feb 25, 2024
6 checks passed
@ksol ksol deleted the feat/naming branch February 25, 2024 19:44
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.

2 participants