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

Public adapter method Delete (and UI component) to be renamed "Remove" #156

Closed
alelom opened this issue Oct 31, 2019 · 2 comments · Fixed by #164
Closed

Public adapter method Delete (and UI component) to be renamed "Remove" #156

alelom opened this issue Oct 31, 2019 · 2 comments · Fixed by #164
Assignees
Labels
type:compliance Non-conforming to code guidelines

Comments

@alelom
Copy link
Member

alelom commented Oct 31, 2019

The "Public Adapter Method" Delete is the method called by the UI Delete component.
It should be renamed to "Remove" or something else that clarifies that it is not a CRUD method per se to avoid confusion.

The confusion is amplified by its default implementation, that only returns a call to the actual CRUD delete:

public virtual int Delete(FilterRequest filter, Dictionary<string, object> config = null)
{
return Delete(filter.Type, filter.Tag);
}

A rename would be beneficial for the following reasons:

  1. It would be consistent with our philosophy of not exposing CRUD methods directly in the UI, unlike now (especially in light of the fact that I deleted the UpdateProperty component for the same reason, which is a good consistency improvement IMHO).
  2. The fact that its default implementation only returns a call to the CRUD delete is only a side-effect of being a default implementation. You can always override the Delete component without affecting the CRUD Delete.
  3. The public method should in principle be called something different than the CRUD Delete because it has all rights to implement additional functionality other than the CRUD Delete strictly speaking
  4. While reviewing the BHoM_Adapter refactoring level03, I realised how confusing is to understand when an Adapter is overriding the actual CRUD Delete or when it's overriding the Delete component method

If we all agree, I will action this in the Level04 refactoring, where other script-breaking changes will happen.

@adecler @IsakNaslundBh @al-fisher @epignatelli

@alelom alelom added the type:question Ask for further details or start conversation label Oct 31, 2019
@alelom alelom self-assigned this Oct 31, 2019
@alelom alelom added the type:compliance Non-conforming to code guidelines label Oct 31, 2019
@adecler
Copy link
Member

adecler commented Nov 1, 2019

While I was not too keen when reading the title, you're making a few good points. As far as I am concerned, it is even more important to make sure that the BHoM is clear and intuitive for the end user than for the few people creating the adapters. As long as "Remove" is well understood and accepted on the end user side, I fine with this idea.

@adecler
Copy link
Member

adecler commented Nov 1, 2019

I think this comment says it all 😄 : BHoM/Mongo_Toolkit#78 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:compliance Non-conforming to code guidelines
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants