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

Unified resource setting method names #1352

Closed
wants to merge 0 commits into from
Closed

Unified resource setting method names #1352

wants to merge 0 commits into from

Conversation

alice-i-cecile
Copy link
Member

As discussed in #1186 and #1349, this PR changes the name of Commands::insert_resource and AppBuilder::add_resource to set_resource.

This is done for two reasons:

  1. Clearly differentiate resource initialization and setting, making it clear that the renamed methods overwrite existing values.
  2. Unify (nearly) identical functionality between the commands and app builder variant.

This should make explaining and learning how to work with Resources easier.

@alice-i-cecile alice-i-cecile changed the title Unified resource setting names Unified resource setting method names Jan 29, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Jan 29, 2021

Could you split the commits into the one changing the names and the one changing the fallout.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Jan 29, 2021

Could you split the commits into the one changing the names and the one changing the fallout.

Do you mean that the first commit should contain only the initial changes within the engine itself, and then the second commit should change the examples?

This change was a simple find-and-replace, so I'm not sure what you mean by "changing the fallout".

@alice-i-cecile
Copy link
Member Author

Discussion on debugging the CI issue here.

@DJMcNab
Copy link
Member

DJMcNab commented Jan 29, 2021

Do you mean that the first commit should contain only the initial changes within the engine itself, and then the second commit should change the examples?

Yeah, the examples and any plugins which use the commands in the second commit.

It all being mixed in one commit makes it difficult to see what the actual changes are.

@cart
Copy link
Member

cart commented Jan 29, 2021

In the interest of unifying terminology, we should make sure each "operation name" is well defined and consistent. Imo these are the preferred definitions/terms:

  • add: adds a new item to a collection that could have duplicates
  • insert: adds or updates an item in a collection without duplicates (the item or key for the item is unique)
  • set: overwrites the current value of a thing that already exits. You "set" fields because they are always there. We might allow "set" in the context of a "collection of unique items", but it should fail if the item does not already exist
  • with: currently a catch all term for builders that "add/insert/set" something. this situation is suboptimal imo

These terms follow rust conventions, which i think is desirable.

Therefore I think this pr should use insert_resource instead of set_resource (which I think we've discussed previously).

@alice-i-cecile
Copy link
Member Author

Right, I remember that conversation now. I agree with your judgement there.

I'll do my best to rebase and fix up this PR this weekend.

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