docs(DB): Add a few docs for database. #230

d-bohls · 2018-02-20T20:36:59Z

Document database/database.py and database/_collection.py.

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).

coveralls · 2018-02-20T20:45:35Z

Coverage remained the same at 68.178% when pulling fad6e51 on d-bohls:databaseDocumentation into ed8b3e9 on ni:master.

coveralls · 2018-02-20T20:45:35Z

Coverage remained the same at 68.178% when pulling ef28adf on d-bohls:databaseDocumentation into ed8b3e9 on ni:master.

jashnani

Thanks for doing this. Some questions and minor concerns:

jashnani · 2018-02-20T20:53:20Z

nixnet/database/_collection.py

+        """Return the database object.
+
+        Args:
+            Name of database object


If we're following our documentation style guide:
key(str): Name of database object.

jashnani · 2018-02-20T21:30:29Z

nixnet/database/database.py

@@ -56,33 +57,89 @@ def __hash__(self):
    def __repr__(self):
        return '{}(handle={})'.format(type(self).__name__, self._handle)

-    def close(self, close_all_refs=False):
+    def close(self, close_all=True):


Why are we changing the default to True? Can you elaborate. This is a breaking API change so we should call it out in the commit.

I was looking at the LV API docs. I'll revert this and follow the C API docs.

jashnani · 2018-02-20T21:36:38Z

nixnet/database/database.py

+        To simplify the task of closing all database objects you opened, you
+        can use the ``close_all`` parameter set to true (default); otherwise,
+        only the database object is closed.
+


This documentation doesn't seem to match the C API docs here. Can you share the source of this content? It seems to be missing some of the info.

I noticed the LabVIEW docs group properties by object (e.g. - the python Cluster class groups properties similarly to a Cluster property node) and thought it might make sense to use LV docs. I now see, we should only use the C API docs.

jashnani · 2018-02-20T21:41:58Z

nixnet/database/_collection.py

            return self._factory(ref)
        else:
-            raise TypeError(index)
+            raise TypeError(key)


I would refrain from making such changes. Rationale is twofold:

We should avoid any variable name changes unless we have a strong reason to do so. Either would've been fine in this case. It adds unnecessary risk and review burden.

This breaks compatibility for users that use param name when passing in value - so we should be minimizing these things.

Same applies to close_all_refs and db_filepath below.

I'll revert, but the only question I have is whether close_all_refs makes sense in the python API, since we don't refer to refs otherwise. Does @epage have any thoughts on this?

Just realized I should add docs for Database __init__ and that the C API docs on nxdbOpenDatabase explain refs in context of opening/closing a database. close_all_refs makes sense.

jashnani · 2018-02-20T21:51:13Z

nixnet/database/database.py

+        path. If opened as an alias, it uses the file path registered for that
+        alias.
+
+        Args:


Can you share the source of this content? We typically use the C API documentation word for word with minor changes/tweaks to make it more "pythonic".

See C API doc here.

Again, LV docs. I'll change to C docs.

I did modify the C API docs for this a bit to exclude info about saving (exporting) a cluster, and I opened issue #231 to add a save method to class Cluster.

jashnani · 2018-02-20T21:58:28Z

nixnet/database/database.py

+        For CANdb (.dbc), LDF (.ldf), or NI-CAN (.ncd) files, the file contains
+        only one cluster, and no cluster name is stored in the file. For these
+        database formats, NI-XNET uses the name Cluster for the single cluster.
+        """


Can you share the source of this content?

d-bohls · 2018-02-21T20:52:02Z

nixnet/database/database.py

+    granted, and the database uses computer resources (memory and handles). For
+    more information, refer to :any:`nixnet.database.database.Database.close`.
+
+    Args:


It may seem strange to put Args here, but I did that because our current Sphinx configuration ignores init docstrings (I created issue #232 ). It looks good on the generated docs though since the class header on readthedocs does show the constructor args.

d-bohls · 2018-02-22T01:58:04Z

docs/conf.py

+
+current_year = datetime.datetime.now().year
+if copyright_year != current_year:
+    warnings.warn('Copyright year is %s. Current year is %s.' % (copyright_year, current_year))


Should this be a warning?
Should we just always use datetime.datetime.now().year in the copyright?

jashnani

Good job on the first pass for documentation. I have some minor comments below.

jashnani · 2018-03-02T22:27:18Z

LICENSE

@@ -1,4 +1,4 @@
-Copyright (c) 2017, National Instruments Corp.
+Copyright (c) 2017-2018, National Instruments Corp.


Thanks for fixing this!

jashnani · 2018-03-02T22:41:29Z

nixnet/database/database.py

+
+    When an already open database is opened,
+    this class grants access to the same database and increases an internal reference counter.
+    A multiple referenced (open) database must be closed as many times as it has been opened.


"A multiple referenced (open) database must be closed as many times as it has been opened."
Should we capture this as a Note in the docstring? And explain that in python upon database object exit, close will get called automatically for them.

jashnani · 2018-03-02T22:54:47Z

nixnet/database/database.py

+
+        If ``db_filepath`` is empty, the file is saved to the same FIBEX file specified when opened.
+        If opened as a file path, it uses that file path.
+        If opened as an alias, it uses the file path registered for that alias.


As discussed in-person we should add any docstrings about references to RT anyway, even if we're not supporting RT right now so we don't have to come back and write those comments.

jashnani · 2018-03-02T22:58:37Z

docs/conf.py

 author = u'National Instruments'
+copyright = u'2017-{}, {}'.format(datetime.now().year, author)


Looks good!

Document database/database.py and database/_collection.py.

Copyright year(s) should now be 2017-2018. Update conf.py so that Sphinx generates doc copyrights that end with the current year.

d-bohls added this to the 0.4 milestone Feb 20, 2018

d-bohls self-assigned this Feb 20, 2018

d-bohls requested a review from jashnani February 20, 2018 20:37

d-bohls force-pushed the databaseDocumentation branch from ef28adf to 196d738 Compare February 20, 2018 21:46

jashnani suggested changes Feb 20, 2018

View reviewed changes

d-bohls force-pushed the databaseDocumentation branch from f79de6f to 3a7ba2d Compare February 21, 2018 20:44

d-bohls commented Feb 21, 2018

View reviewed changes

d-bohls force-pushed the databaseDocumentation branch 2 times, most recently from fd5840f to dace8cf Compare February 22, 2018 01:24

d-bohls commented Feb 22, 2018

View reviewed changes

d-bohls force-pushed the databaseDocumentation branch 3 times, most recently from 5042b52 to f156042 Compare February 27, 2018 23:09

jashnani suggested changes Mar 2, 2018

View reviewed changes

d-bohls added 2 commits March 5, 2018 17:04

docs(DB): Add a few docs for database.

f26dde4

Document database/database.py and database/_collection.py.

docs: Update copyright info.

fad6e51

Copyright year(s) should now be 2017-2018. Update conf.py so that Sphinx generates doc copyrights that end with the current year.

d-bohls force-pushed the databaseDocumentation branch from 6cad32b to fad6e51 Compare March 5, 2018 23:05

jashnani approved these changes Mar 5, 2018

View reviewed changes

jashnani merged commit 730305b into ni:master Mar 5, 2018

d-bohls deleted the databaseDocumentation branch March 5, 2018 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(DB): Add a few docs for database. #230

docs(DB): Add a few docs for database. #230

d-bohls commented Feb 20, 2018

coveralls commented Feb 20, 2018 •

edited

Loading

coveralls commented Feb 20, 2018

jashnani left a comment

jashnani Feb 20, 2018

jashnani Feb 20, 2018

d-bohls Feb 20, 2018

jashnani Feb 20, 2018

d-bohls Feb 20, 2018

jashnani Feb 20, 2018

d-bohls Feb 20, 2018 •

edited

Loading

d-bohls Feb 20, 2018

jashnani Feb 20, 2018

d-bohls Feb 20, 2018 •

edited

Loading

d-bohls Feb 21, 2018

jashnani Feb 20, 2018

d-bohls Feb 21, 2018

d-bohls Feb 22, 2018

jashnani left a comment

jashnani Mar 2, 2018

jashnani Mar 2, 2018

jashnani Mar 2, 2018

jashnani Mar 2, 2018

		@@ -1,4 +1,4 @@
		Copyright (c) 2017, National Instruments Corp.
		Copyright (c) 2017-2018, National Instruments Corp.

		author = u'National Instruments'
		copyright = u'2017-{}, {}'.format(datetime.now().year, author)

docs(DB): Add a few docs for database. #230

docs(DB): Add a few docs for database. #230

Conversation

d-bohls commented Feb 20, 2018

coveralls commented Feb 20, 2018 • edited Loading

coveralls commented Feb 20, 2018

jashnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-bohls Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-bohls Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jashnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 20, 2018 •

edited

Loading

d-bohls Feb 20, 2018 •

edited

Loading

d-bohls Feb 20, 2018 •

edited

Loading