ensure oid on edit method is not flagged by kwargs_match_entity

[calebsyring]

The @kwargs_match_entity decorator used on the edit method on MethodsMixin https://github.com/level12/keg-elements/blob/168e70af2b49bfd3f1e97705b86d158821e5d2ad/keg_elements/db/mixins.py#L193 will throw an error when oid is passed in as a kwarg (because oid is not a column or relationship on the entity). We don't catch this in the tests because we pass the oid in without the keyword: https://github.com/level12/keg-elements/blob/168e70af2b49bfd3f1e97705b86d158821e5d2ad/keg_elements/tests/test_db_mixins.py#L77

I changed the oid kwarg to _oid in the method definition because the decorator ignores kwargs starting with an underscore. This shouldn't be a breaking change because anyone passing in oid using the kwarg would have been getting an error anyway (unless they happened to have an oid column/relationship on the entity).

[rsyring]

oid was never intended to be passed as a keyword argument. In a Python 3.8+ world, I would have made it a position-only argument.

    def edit(cls, oid=None, /, **kwargs):
        """Edit an object in session with the kwargs, and optionally flush or commit. """

I'm not sure if we only want to support Python 3.8+, @level12/devs have any thoughts on that?

@calebsyring I think the correct action here is to change the test back to how it was. Was there a specific reason you changed it to call update() w/ the keyword argument?

[calebsyring]

Was there a specific reason you changed it to call update() w/ the keyword argument?

@rsyring primarily because there was a bug, and I wanted the tests to fail to show it before I fixed the bug (TDD).

If we're keeping 3.7 support, I could revert the change and add an additional test using the kwarg with a comment like "this is not how the method is intended to be used, but we need to test the case because it is possible it will be used that way".

[rsyring]

It's kind of an interesting case for figuring out how we want to test this. The "bug" was a BC issue with the change Matt made BUT only if the method is called in a way that it was never intended to be called, although, as you point out, it is possible to be called that way and the resulting error message is really a red herring.

If we are staying at 3.7, I think the change to _oid is ok. It might be better to update kwargs_match_entity to indicate excluded variables:

@kwargs_match_entity(exclude='oid')
@ classmethod
def edit(self, oid=None, **kwargs):
   # ....

Well, maybe it's better to keep the implementation simple since as soon as we can go to 3.8 we can use the positional only method definition to indicate our intended behavior. Although, now that I'm thinking about it, even if we do that, I bet the kwargs_match_entity decorator is going to fail with the misleading error message before the method is called.

My guess is that in 3.8 kwargs_match_entity could inspect the method signature and include any positional-only arguments from it's check. That makes the decorator a bit more magical but also means we don't have to manually update the decorator everywhere it's used to take into account the excluded argument name.

@guruofgentoo any preference or alternative?