feat(API): Adding partial database API support#169
Conversation
|
Will be posting an update shortly with slight different design (using an alias object). |
| database_alias2 = 'custom_database2' | ||
| database_filepath2 = '../databases/custom_database2.dbc' | ||
| default_baud_rate2 = '250000' | ||
| my_system.aliases[database_alias2] = (database_filepath2, default_baud_rate2) |
There was a problem hiding this comment.
I don't think adding through assignment will work for an API like this. That implies that you'll get the same object back on lookup but we can't implement that. My recommendation is an explicit add function
There was a problem hiding this comment.
resolved. See also PR description.
| database_alias3 = 'custom_database3' | ||
| database_filepath3 = '../databases/custom_database3.dbc' | ||
| default_baud_rate3 = '800000' | ||
| my_system.aliases[database_alias3] = (database_filepath3, default_baud_rate3) |
There was a problem hiding this comment.
aliases doesn't give context to the user of what this is meant to be. db_aliases or database_aliases would though.
| output_session.frames.write([frame]) | ||
| print('Sent frame with ID %s payload: %s' % (id, payload)) | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
I'm assuming this is more so to showcase the API work you are doing rather than an example because I wouldn't expect an example to be creating multiple aliases.
There was a problem hiding this comment.
No, I was expecting us to create multiple aliases and use them in the example. @lilesj what are your thoughts on this? are there other uses cases/workflows I should consider writing examples for?
There was a problem hiding this comment.
Demonstrating how to parse a folder for DB files is an important use case. We might want to change one of the database file to a fibex file to show we can do it with multiple file types. My comment below mentions how we can reinforce the purpose of the example by reordering the operations a bit.
| elif terminated_cable.lower() == "n": | ||
| output_session.intf.can_term = constants.CanTerm.ON | ||
| else: | ||
| print("Unrecognised input ({}), assuming 'n'".format(terminated_cable)) |
There was a problem hiding this comment.
If you're making this a function distinct function then it shouldn't take user input but should receive all parameters from the caller.
| @@ -0,0 +1,91 @@ | |||
| from __future__ import absolute_import | |||
There was a problem hiding this comment.
programmatic_database_api.py
The name feels very awkward to me. Is there an equivalent C or LV example for what this is trying to accomplish? What is it named? What does it look like / do?
There was a problem hiding this comment.
Agreed, the functionality is limited to adding and removing aliases for the moment so the name should probably reflect that in some way.
There was a problem hiding this comment.
I couldn't find an equivalent example for what I'm trying to do. There is a C example called XNET_Database_Browser but it doesn't leverage add or remove alias. It might be good for the get database list or get database list size functions that I'm writing.
I trying to demonstrate that the same read/write code can be reused with different database aliases (different cluster, baud rates, etc.). This sounded like a common use case for aliases based on my conversation with Jeff.
How about dynamic_database_usage.py or programmatic_database_usage.py?
There was a problem hiding this comment.
This hasn't been renamed yet
2 similar comments
| alias3 = Alias(database_alias3, database_filepath3, default_baud_rate3) | ||
| my_system.aliases[alias3.database_name] = alias3 | ||
| print(list(my_system.aliases)) | ||
|
|
There was a problem hiding this comment.
Since the intent of the example is to demonstrate the use of the database functions, can we call the alias functions first to show they are being created in preparation for the session objects.
|
|
||
| def write_can_frame_using_alias(database_name): | ||
| def write_can_frame_using_alias(alias): | ||
| database_name = alias.database_name |
There was a problem hiding this comment.
What should the property be called?
It seems weird to mix alias / name terminology.
We could just implement __str__ and expect people to use that (and the session API can be updated to accept both str and Alias)
| default_baud_rate1 = '500000' | ||
| my_system.aliases[database_alias1] = (database_filepath1, default_baud_rate1) | ||
| alias1 = Alias(database_alias1, database_filepath1, default_baud_rate1) | ||
| my_system.aliases[alias1.database_name] = alias1 |
There was a problem hiding this comment.
Again, we have an asymmetry problem. We give NI-XNET more information than we can lookup.
I suspect it'll be cleaner and cause us fewer short term problems if we have an add function that takes the parameters and makes the call. Then when you do my_system.aliases["database_alias1"], you get back and Alias object that only has database_name (or whatever we call it) on it.
2 similar comments
| database_alias = 'custom_database' | ||
| database_filepath = '../databases/custom_database.dbc' | ||
| default_baud_rate = '500000' | ||
| my_system.databases.add(database_alias, database_filepath, default_baud_rate) |
There was a problem hiding this comment.
I thought we were leaning towards this being called add_alias?
There was a problem hiding this comment.
I changed it in the implementation but forgot to update the example.
| elif terminated_cable.lower() == "n": | ||
| output_session.intf.can_term = constants.CanTerm.ON | ||
| else: | ||
| print("Unrecognised input ({}), assuming 'n'".format(terminated_cable)) |
|
|
||
| def main(): | ||
| with system.System() as my_system: | ||
| print(list(my_system.databases)) |
There was a problem hiding this comment.
Hmm, I'm torn.
Technically, when you iterate on a mapping, you get back the keys rather than the values.
DAQmx implemented a sequence that also accepts strings. Not sure how well that fits here.
I'd say carry on but this is something we need to eventually resolve.
There was a problem hiding this comment.
Opened #173 about this.
The interesting aspects for this situation
- Can we support integer indexes? This requires the database list returned by the driver to be a stable order
- Is it ok if we still implement get/del? Since get is just a function, probably. del is fine; it works even on mutable sequences (granted, we won't have a fully mutable sequence).
|
Shortly, I will post a separate commit with unit/system tests that should go along with this feature. |
| import collections | ||
| import itertools | ||
| import six | ||
| import typing # NOQA: F401 |
There was a problem hiding this comment.
six is a third party module, please have it in a separate section
nixnet/_funcs.py
Outdated
| _errors.check_for_error(result.value) | ||
| alias_list = alias_buffer_ctypes.value.decode("ascii").split(",") | ||
| filepath_list = filepath_buffer_ctypes.value.decode("ascii").split(",") | ||
| return list(zip(alias_list, filepath_list)) |
There was a problem hiding this comment.
We've generally left post-processing like this to the clients of _funcs
There was a problem hiding this comment.
I left the decoding of the strings logic in this layer thinking that this would need to be generated for all string output types anyway.
I've moved the 'split' and 'zip' logic to the layer above (databases) though.
nixnet/_funcs.py
Outdated
| ): | ||
| # type: (...) -> typing.Tuple[List[typing.Text], List[typing.Text], int] | ||
| ip_address_ctypes = _ctypedefs.char_p(ip_address.encode('ascii')) | ||
| size_of_alias_buffer, size_of_filepath_buffer = nxdb_get_database_list_sizes(ip_address) |
There was a problem hiding this comment.
Generally we leave policy like this to the callers of _funcs. If, in the future, we add code-gen, it'll make life a lot easier
There was a problem hiding this comment.
I've cleaned this up and move the call the sizes function one layer above in databases.py
nixnet/system/databases.py
Outdated
| return len(_funcs.nxdb_get_database_list('')) | ||
|
|
||
| def __iter__(self): | ||
| for alias, filepath in _funcs.nxdb_get_database_list(''): |
There was a problem hiding this comment.
for alias, _ in ...
_ is a generally recommended placeholder for "don't care" values
nixnet/system/databases.py
Outdated
| else: | ||
| raise ValueError('Database alias %s not found in the system' % index) | ||
| else: | ||
| raise KeyError(index) |
There was a problem hiding this comment.
I might not have been clear before.
We should throw KeyError when the key isn't found.
As for what to throw on unsupported types, it looks like we've set the precedence of TypeError
https://github.com/ni/nixnet-python/blob/master/nixnet/_session/collection.py#L44
nixnet/system/databases.py
Outdated
| database_filepath, | ||
| ): | ||
| self._handle = handle | ||
| self._index = index |
There was a problem hiding this comment.
Why does this take a handle or index?
There was a problem hiding this comment.
Got rid of them, don't need them anymore.
| self._handle = handle | ||
| self._index = index | ||
| self._database_alias = database_alias | ||
| self._database_filepath = database_filepath |
There was a problem hiding this comment.
Don't we need to define a property to expose filepath?
There was a problem hiding this comment.
Yes, I've a added a property for this. Just the get function for now.
| @@ -0,0 +1,91 @@ | |||
| from __future__ import absolute_import | |||
There was a problem hiding this comment.
This hasn't been renamed yet
| print(list(my_system.databases.keys())) | ||
| print(list(my_system.databases.values())) | ||
| print(list(my_system.databases.items())) | ||
| print(len(my_system.databases)) |
There was a problem hiding this comment.
Didn't we discuss removing all these prints? This is an example and not a test
There was a problem hiding this comment.
Yes, I just forgot to take them out. Good catch.
| print(list(my_system.databases.items())) | ||
| print(len(my_system.databases)) | ||
| database_alias = 'custom_database' | ||
| database_filepath = 'databases/custom_database.dbc' |
There was a problem hiding this comment.
Probably want to prefix this with basename(__file__) or whatever the call would be so they can run this from other locations
There was a problem hiding this comment.
This doesn't look like its resolved.
There was a problem hiding this comment.
Oops, I posted another comment on this.
jashnani
force-pushed
the
databaseAPI2
branch
6 times, most recently
from
376b3ff to
28ffe2a
Compare
epage
changed the title
2 similar comments
jashnani
force-pushed
the
databaseAPI2
branch
2 times, most recently
from
22e290d to
cb7e7f8
Compare
| from nixnet import _funcs | ||
|
|
||
|
|
||
| class Databases(collections.Mapping): |
tests/test_system.py
Outdated
| print(list(sys.databases)) | ||
| num_databases = len(sys.databases) | ||
| database_alias = 'test_database' | ||
| database_filepath = os.path.join(os.path.dirname(__file__), '..', 'nixnet_examples\databases\custom_database.dbc') |
There was a problem hiding this comment.
For clean up after this, we should have another dirname rather than ..
jashnani
force-pushed
the
databaseAPI2
branch
2 times, most recently
from
4540bca to
c323383
Compare
1 similar comment
4 participants
toxsuccessfully runs, including unit tests and style checks (see CONTRIBUTING.md).What does this Pull Request accomplish?
Adds partial database API support.
Why should this Pull Request be merged?
Enhances the API with programmatic database functionality. This fixed #77.
What testing has been done?
The example runs as expected. Integration and unit tests run as expected.
Design Alternatives
Goals
DatabaseAlias Mapping
(current design)
System.databasesis a customMappingobject with Alias objects that store the results. These objects could also have an "open database" call.iter, keys, values, items, len, getitem, delitem all work as expected.
setitem could be implemented. There are a couple of annoyances
DatabaseAliasobject would need to be created by the user and passed in.Alias/Path Mapping
System.databasesis a customMappingobject from alias to file path. Everything else is similar toDatabaseAlias Mapping.Distinct Functions (no ABC)
Whether
Systemfunctions or free functions (which would mean they could take the IPAddress for RT support).Known Issues
System.databases property is missing a type check because flake8 is throwing an error when the return type is set to 'databases.Databases'. I suspect this might have to do with 'databases' being overused.