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

Consider removing Deleter interface and always use .=id=X to delete item #173

Open
1 task done
maksym-nazarenko opened this issue Jun 17, 2023 · 1 comment
Open
1 task done
Labels
question Further information is requested

Comments

@maksym-nazarenko
Copy link
Collaborator

maksym-nazarenko commented Jun 17, 2023

It looks like that =numbers=X is a convenience way to delete items not only by ID, but also by name, id, etc.
As we moving forward with new interface-based implementation for all client resources, we could consider always using item's .id field to remove it.

It would simplify logic and remove some unnecessary code.

Find<ResourceName>() functions in client would need to be updated to Find<ResourceName>ByName() (in case of name attribute) so underlying helper function will:

  1. Find the remote MikroTik object by its field.
  2. Get .id from the record and call Delete() generic method.

However, it will still be possible to delete particular instance of the object, if you have one, e.g:

c := NewClient(GetConfigFromEnv())
list, _ := c.FindInterfaceList("list_name")
c.Delete(list)

TODO:

  • Check if all tests pass in case of the change

@ddelnano wdyt?

@maksym-nazarenko maksym-nazarenko added the question Further information is requested label Jun 17, 2023
@maksym-nazarenko
Copy link
Collaborator Author

@ddelnano I updated all client tests to use generic client.Delete(...) function which accepts Resource interface and uses .id field to delete RouterOS resource and everything worked.

One thing I noticed was that we have delete functions accepting other unique identifiers like name, e.g. c.DeleteInterfaceList("list-name") so changing those functions to accept id field would break the things.

At this moment I am not sure if it is feasible to proceed with this change and we can leave it as-is as it brings no benefits 🤷
wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant