feat(API): Add descendant find for parent db objects. by d-bohls · Pull Request #248 · ni/nixnet-python

d-bohls · 2018-04-11T20:57:15Z

Fixes #239

The following parent database objects now have a find method:

database
cluster
lin schedule
pdu
frame
subframe

All database object are now subclasses of
DatabaseObject, which is used as an interface.
Database objects are now exposed through the
nixnet.database namespace, and their constructors
now take kwargs to signal to users that these
objects should not be instantiated by users.

I tested on the shipping example database "NIXNET_example"
to ensure I can indeed find children and grandchildren,
and that sane errors are thrown when the object name
not found or the object class is wrong.
Unit tests will be added in #238.

This contribution adheres to CONTRIBUTING.md.
~~New tests have been created for any new features or regression tests for bugfixes.~~
tox successfully runs, including unit tests and style checks (see CONTRIBUTING.md).

d-bohls · 2018-04-11T21:00:39Z

A couple of notes on this review.

After #237 goes in, I plan to add docstrings to this review, because the docstrings for this review will reference docstrings in the other review.

I am deferring unit tests for this review to be done as part of #238.

coveralls · 2018-04-11T21:02:18Z

Coverage increased (+0.1%) to 67.679% when pulling 263b1ae on d-bohls:dbFindObject into fba4880 on ni:master.

coveralls · 2018-04-11T21:02:18Z

Coverage decreased (-0.3%) to 67.914% when pulling f3d3ceb on d-bohls:dbFindObject into 82aad3b on ni:master.

coveralls · 2018-04-11T21:02:18Z

Coverage decreased (-0.3%) to 67.914% when pulling f3d3ceb on d-bohls:dbFindObject into 82aad3b on ni:master.

coveralls · 2018-04-11T21:02:18Z

Coverage decreased (-0.3%) to 67.914% when pulling f3d3ceb on d-bohls:dbFindObject into 82aad3b on ni:master.

d-bohls · 2018-04-11T21:20:02Z

Why you spamming me @coveralls!

epage · 2018-04-13T15:10:24Z

Would it be possible to post a comment of what your modified example looks like, so we can see what the API is like to use? Or more ideally, tests :)

epage · 2018-04-13T15:18:31Z

nixnet/database/_find_object.py

+        _errors.check_for_error(_cconsts.NX_ERR_INVALID_PROPERTY_VALUE)
+        # The above line will raise an exception.
+        # However, mypy doesn't know that and will complain if the following return is missing.
+        return None


Looks like you could make the return type a union of the regular and NoReturn

epage · 2018-04-13T15:23:39Z

nixnet/database/_find_object.py

+
+
+def find_object(parent_handle, object_class, object_name):
+    # type: (int, constants.ObjectClass, typing.Text) -> object


Instead of an enum, should people pass Cluster etc into here?

Wish mypy had dependent types. That'd be cool to say that the return type is the type of object_class.

Regarding 1, I had thought about doing it that way. I guess on the plus side, it eliminates an enum the user has to deal with, and encourages familiarity with the actual objects. On the con side, those of us who use IDEs lose the benefit of intellisense for the enum values. I think my preference would be to continue using the enum as I don't really see much benefit to passing in an object instead of an enum. I'm open to either way though.

On the con side, those of us who use IDEs lose the benefit of intellisense for the enum values.

Completion for nixnet.constants.ObjectClass is not much different than completion for nixnet.database.

As for other intellisense features, you can improve that by

Providing a common base class for all database objects

Change the annotations from type: (int, constants.ObjectClass, typing.Text) -> object to type: (int, typing.Type[nixnet.database.SomeBaseClass], typing.Text) -> nixnet.database.SomeBaseClass

This is similar to the approach we took elsewhere in the API

Define FrameFactory have have each frame implement it

Accept a frame object when reading and converting

I think my preference would be to continue using the enum as I don't really see much benefit to passing in an object instead of an enum.

Advantages

Minimize vocabulary terms the user of the API has to deal with. I need a Cluster, so let me pass a Cluster vs having to deal with an enum that represents Cluster.

Avoid C-isms. Part of the role of the enum is a form of RTTI in C. Changing it to the actual python class is exposing the same concept in the python way

Controlled access. The doc string and intellisense will help guide people to the correct values to pass in rather than having things like System be an option.

d-bohls · 2018-04-13T18:24:07Z

@epage As requested, here is some code to show usage of find.

import nixnet
from nixnet.constants import ObjectClass
from nixnet import database


def main():
    with database.Database("NIXNET_example") as db:
        cluster = db.find(ObjectClass.CLUSTER, "CAN_Cluster")
        print(cluster.name)
        frame1 = db.find(ObjectClass.FRAME, "CANCyclicFrame2")
        frame2 = cluster.find(ObjectClass.FRAME, "CANCyclicFrame2")
        print(frame1.name)
        print(frame2.name)
        assert (frame1 == frame2)
        signal = db.find(ObjectClass.SIGNAL, "ClutchPressure")
        print(signal.name)

epage · 2018-04-13T18:30:41Z

For reference, with using classes instead of enums

import nixnet
from nixnet import database


def main():
    with database.Database("NIXNET_example") as db:
        cluster = db.find(database.Cluster, "CAN_Cluster")
        print(cluster.name)
        frame1 = db.find(database.Frame, "CANCyclicFrame2")
        frame2 = cluster.find(database.Frame, "CANCyclicFrame2")
        print(frame1.name)
        print(frame1.name)
        assert (frame1 == frame2)
        signal = db.find(database.Signal, "ClutchPressure")
        print(signal.name)

Oh, fun, that won't work as of this moment. nixnet.database only exposes Database and not the other types
https://github.com/ni/nixnet-python/blob/master/nixnet/database/__init__.py

Interesting. I know the concern was about exposing the useless private constructors to users but this doesn't allow our users to use mypy.

A workaround for exposing the useless private constructors is to make handle a keyword-only arg that is prefixed with _. That isn't officially supported in python2 but I recently learned about a backport of the feature: https://pypi.python.org/pypi/kwonly-args

jashnani

I have a preference for using classes instead of enums, mainly because I don't have to remember the enum name. Other reasons mentioned by @epage are also compelling.

Exposing useless private constructors can be tricky and I'm curious what that would look like to the user. If the type/class is completely useless and that's apparent to the user, I would be okay with this approach.

d-bohls · 2018-04-19T05:44:50Z

I'm curious about your reasoning for suggesting kwonly-args and not kwargs? Won't both work fine for this purpose? kwonly-args looks like it might be a cleaner implementation.

epage · 2018-04-19T15:03:38Z

I'm curious about your reasoning for suggesting kwonly-args and not kwargs? Won't both work fine for this purpose? kwonly-args looks like it might be a cleaner implementation.

If you are taking about usage, any keyword argument can also be a positional argument.

If you are talking about **kwargs, then that is effectively another way of defining a keyword-only argument and fits the spirit of what I'm suggesting. The literal keyword-only argument feature just requires less boiler plate / is more clear on intent.

d-bohls · 2018-04-25T07:09:31Z

I've done the following:

Introduced DatabaseObjectBase for all database object to inherit from
Exposed them in database package's init
Modified find methods to use DatabaseObjectBase objects

However, I'm still not seeing a useful namespacing experience. Not sure yet what I'm missing.

I'm holding off on producing docstrings and making the kwargs constructor changes until I can fix the namespacing experience.

epage · 2018-04-25T13:49:32Z

nixnet/database/_find_object.py

+        class_enum = constants.ObjectClass.SUBFRAME
+    else:
+        # arbitrary value for class_enum so mypy sees a value in each condition
+        class_enum = constants.ObjectClass.CLUSTER


I'd actually recommend a raise ValueError("Unsupported database object")

Oh, missed the raise below :)

epage · 2018-04-25T13:52:16Z

nixnet/database/__init__.py



-__all__ = ["Database"]
+__all__ = [


DatabaseObjectBase should be in here so users can do mypy typing

epage · 2018-04-25T13:53:05Z

nixnet/database/_database_object_base.py

+import typing  # NOQA: F401
+
+
+class DatabaseObjectBase(object):


Since this should be public, we should ensure we have a name we like. I personally find -Base suffixes a little off, at least in python code.

I was following what I saw with SessionBase. So your suggestion is DatabaseObject?

iirc SessionBase was me doing what I said not to do, concrete inheritance for code sharing. Its not actually exposed in the API.

Something like DatabaseObject might work.

epage · 2018-04-25T13:53:58Z

I've done the following:

Looks slick

However, I'm still not seeing a useful namespacing experience. Not sure yet what I'm missing.

What do you mean?

d-bohls · 2018-04-25T20:13:43Z

@epage

However, I'm still not seeing a useful namespacing experience. Not sure yet what I'm missing.

What do you mean?

Apologies for not being clear there. I was talking about the intellisense issue where I don't see anything useful in the database namespace after typing "database.".

Turns out, this seems to be a PyCharm quirk where my nixnet-python package was defined in my PyCharm project (rather than being installed traditionally) and using a loose python script outside of the project to test with. It's strange because the script would run just fine, but I wasn't getting any intellisense for things in nixnet. Once I include my test files as a source root in the project, the intellisense automagically works.

d-bohls · 2018-04-26T04:51:40Z

Added docstrings and modified database objects to init with kwargs.

@jashnani and I chose to not include docs for the new DatabaseObject base class.

Thanks to @jashnani and @epage for not settling on "good enough". I really like the final design.

I'll add tests for this and all database objects next.

d-bohls · 2018-04-26T04:53:24Z

@jashnani Just in case, let me squash before merge.

d-bohls · 2018-04-26T06:03:50Z

The only "less than ideal" issue I might mention is that the DatabaseObject base class shows up in intellisense, though this is better than the enum alone since other things like Session showed up in that.

jashnani

Looks good overall! Keyword args seem to do the trick and intellisense is working.

jashnani · 2018-04-26T20:16:51Z

nixnet/database/_cluster.py

+        *   :any:`SubFrame`
+        *   :any:`Ecu`
+        *   :any:`LinSched`
+        *   :any:`LinSchedEntry`


Would it be possible to have fully qualified names of accepted values while keeping links to the classes? For ex:
nixnet.database.Cluster
nixnet.database.Frame
nixnet.database.Signal
etc.

But have the links go to the appropriate class RTD page.

jashnani · 2018-04-26T20:24:33Z

nixnet/database/_database_object.py

+    ):
+        # type: (...) -> None
+        if not kwargs or '_handle' not in kwargs:
+            _errors.raise_xnet_error(_cconsts.NX_ERR_INVALID_PROPERTY_VALUE)


We should raise a TypeError similar to keyword-only saying more arguments were passed in than expected.
For ex: TypeError: () takes exactly 1 argument (2 given)

An alternative would be to switch to using keyword-only args which will automatically throw this error us.

You mentioned this PEP for keyword-only arguments:

https://www.python.org/dev/peps/pep-3102/

"In accordance with the current Python implementation, any errors encountered will be signaled by raising TypeError."

Changed to raise TypeError(),

jashnani · 2018-04-26T21:01:14Z

nixnet/database/_signal.py

    def pdu(self):
        # actually returns _pdu.Pdu, but avoiding a circular import
-        # type: () -> typing.Any
+        # type: () -> _database_object.DatabaseObject


Should the return type just be PDU? Ditto as above

jashnani · 2018-04-27T16:28:30Z

nixnet/database/_find_object.py

+        class_enum = constants.ObjectClass.SIGNAL
+    elif object_class is SubFrame:
+        class_enum = constants.ObjectClass.SUBFRAME
+    else:


If you're wishing there was a switch-statement in Python, there's a convention for using a dictionaries in situations like this. Where keys would be object _classes and values would be enum_classes.

Here's an example: https://github.com/ni/nixnet-python/blob/master/nixnet/types.py#L1022

I had thought about using a dictionary but had no strong preference.

It's a dictionary now. If the key isn't found, I'm raising a ValueError for the object_class argument.

jashnani · 2018-04-27T16:32:00Z

nixnet/database/_signal.py

    def frame(self):
        # actually returns _frame.Frame, but avoiding a circular import
-        # type: () -> typing.Any
+        # type: () -> _database_object.DatabaseObject


Shouldn't the return type just be Frame? Could we workaround the circular import by scoping it in this property?

Shouldn't the return type just be Frame?

Yes, that's what the comment is saying. I couldn't find a way to import before the type check, and not for lack of trying.

There's a way to do it by setting MYPY = false. See details here: https://mypy.readthedocs.io/en/latest/common_issues.html#import-cycles

I'm not going to hold the review for this, so don't feel like you have to go out of way to do this.

I was able to get this to work using the MYPY = False technique. Cool.

jashnani · 2018-04-27T16:32:30Z

nixnet/database/_subframe.py

    def pdu(self):
        # actually returns _pdu.Pdu, but avoiding a circular import
-        # type: () -> typing.Any
+        # type: () -> _database_object.DatabaseObject


Should the return type just be PDU? Ditto as above.

I was able to get this to work using the MYPY = False technique.

jashnani · 2018-04-27T16:37:30Z

nixnet/database/_collection.py

        if isinstance(index, six.string_types):
            ref = _funcs.nxdb_find_object(self._handle, self._type, index)
-            return self._factory(ref)
+            return self._factory(_handle=ref)


DBCollection init function's factory parameter type can now be changed to DatabaseObject
# type: (int, constants.ObjectClass, int, typing.Type[_database_object.DatabaseObject]) -> None

jashnani · 2018-04-27T18:38:03Z

nixnet/database/_database_object.py

+        return hash(self._handle)
+
+    def __repr__(self):
+        return '{}(handle={})'.format(type(self).__name__, self._handle)


Our use of DatabaseObject is meant for interface only. I would've expected these implementations to continue living in child classes and evolve as necessary:

__eq__
__ne__
__hash__
__repr__

I read the information you sent me on this. I do understand some of the benefits mentioned to not DNR (like code locality). In this particular situation, I think I'd still prefer not to duplicate theses four functions in all eight subclasses. Regardless, I've moved them back as they were and I'll add unit tests as appropriate in my upcoming PR.

jashnani · 2018-04-27T18:43:28Z

nixnet/database/_signal.py

    def mux_subfrm(self):
        # actually returns _subframe.SubFrame, but avoiding a circular import
-        # type: () -> typing.Any
+        # type: () -> _database_object.DatabaseObject


Should the return type just be SubFrame? Ditto as above.

I was able to get this to work using the MYPY = False technique.

Fixes ni#239 The following parent database objects now have a find method: - database - cluster - lin schedule - pdu - frame - subframe All database object are now subclasses of DatabaseObject, which is used as an interface. Database objects are now exposed through the nixnet.database namespace, and their constructors now take kwargs to signal to users that these objects should not be instantiated by users. I tested on the shipping example database "NIXNET_example" to ensure I can indeed find children and grandchildren, and that sane errors are thrown when the object name not found or the object class is wrong. Unit tests will be added in ni#238.

d-bohls added the enhancement label Apr 11, 2018

d-bohls self-assigned this Apr 11, 2018

d-bohls requested review from epage and jashnani April 11, 2018 20:57

epage reviewed Apr 13, 2018

View reviewed changes

jashnani reviewed Apr 16, 2018

View reviewed changes

jashnani self-requested a review April 16, 2018 19:31

d-bohls force-pushed the dbFindObject branch 4 times, most recently from fed7115 to d381e6e Compare April 25, 2018 06:44

epage reviewed Apr 25, 2018

View reviewed changes

d-bohls force-pushed the dbFindObject branch from 7e39355 to fe67f88 Compare April 25, 2018 20:49

d-bohls force-pushed the dbFindObject branch 2 times, most recently from db473e2 to 5e664b9 Compare April 26, 2018 05:44

d-bohls force-pushed the dbFindObject branch from 6e1de5a to b472b32 Compare April 27, 2018 15:05

jashnani reviewed Apr 27, 2018

View reviewed changes

d-bohls force-pushed the dbFindObject branch 3 times, most recently from fc76719 to 6b3ebde Compare April 30, 2018 18:53

jashnani approved these changes Apr 30, 2018

View reviewed changes

d-bohls added 2 commits April 30, 2018 16:11

fix(docs): fix docs namespace for lin_sched

263b1ae

d-bohls force-pushed the dbFindObject branch from b440e94 to 263b1ae Compare April 30, 2018 21:11

jashnani merged commit 0132e8c into ni:master Apr 30, 2018

d-bohls deleted the dbFindObject branch April 30, 2018 21:46



		def find_object(parent_handle, object_class, object_name):
		# type: (int, constants.ObjectClass, typing.Text) -> object



		__all__ = ["Database"]
		__all__ = [

Conversation

d-bohls commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-bohls commented Apr 11, 2018

Uh oh!

coveralls commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 11, 2018

Uh oh!

coveralls commented Apr 11, 2018

Uh oh!

coveralls commented Apr 11, 2018

Uh oh!

d-bohls commented Apr 11, 2018

Uh oh!

epage commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-bohls commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Apr 13, 2018

Uh oh!

jashnani left a comment

Choose a reason for hiding this comment

Uh oh!

d-bohls commented Apr 19, 2018

Uh oh!

epage commented Apr 19, 2018

Uh oh!

d-bohls commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Apr 25, 2018

Uh oh!

d-bohls commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-bohls commented Apr 26, 2018

Uh oh!

d-bohls commented Apr 26, 2018

Uh oh!

d-bohls commented Apr 26, 2018

Uh oh!

jashnani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-bohls commented Apr 11, 2018 •

edited

Loading

coveralls commented Apr 11, 2018 •

edited

Loading

d-bohls commented Apr 13, 2018 •

edited

Loading

d-bohls commented Apr 25, 2018 •

edited

Loading

jashnani left a comment •

edited

Loading

d-bohls Apr 30, 2018 •

edited

Loading