Class inheritance and docstrings

[dmdunla]

Fixing minor issues: all classes inheriting from object, copyright notice, more complete testing, removing random docstrings

[ntjohnson1]

The random doc strings were intended to be module level doc strings that print out on help when importing the module but not the contained class. I never checked them.

Also in python3 object as a base is implied so I think we should probably go the other direction of removing object universally. https://docs.python.org/3/tutorial/classes.html and https://www.python.org/doc/newstyle/

I thought both of these were enforced by pylint but testing seems to have passed. As long as testing passes low priority.

[dmdunla]

The non-explicit object inheritance caused problems with coveralls coverage. The file-level docstrings were repeated info, but we can put them back if there is interest. Not all classes had them so I was trying to be consistent.

[ntjohnson1]

Interesting. I wonder if that relates to coveralls backwards compatibility with python 2 still. No strong opinions on it since our current code layout is basically one module per tensor type implementation. Can revisit if we shuffle the code around post 2.0. I think our flat structure is mostly just to match the MATLAB version.

[dmdunla]

@ntjohnson1 How do you view docs? Just trying to get some consensus before moving forward on trying to move towards consistency.

Also, I would be happy to revisit code structure after 2.0.

[ntjohnson1]

In decreasing frequency:

• In the code
• python command line when I am trying something out help(sptensor)
• readthedocs

I just pulled your PR locally and ran pytest tests/ --cov=pyttb --cov-report=term-missing --packaging and pylint was upset for missing module doc strings on all these files. If we find them redundant or unnecessary we can disable that pylint rule. I am surprised you didn't hit this locally or in CI. Maybe my pylint is old and this has changed.
https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/missing-module-docstring.html

I also got useless-object-inheritance for the introduced objects.

[ntjohnson1]

Note that the whole discussion here is part of the value of the linter. We can just defer to that. I've just been working on other pieces of pyttb. But if I can push to get complete coverage with that then its hopefully something we never need to think about again. I noticed here we weren't currently enforcing the partial pylint coverage I already added. So let me try to open a draft PR to clean things up and at least pin our progress.

[dmdunla]

I had a problem with an older version of black that was barfing on some lines. I updated to the version installed in the CI (regression) tests in 3.8, 3.9, and 3.10. Perhaps we are at the point where we need to include a requirements.txt file for our dev ebb so that we are all using the same local dependency versions.

…

________________________________ From: Nick ***@***.***> Sent: Monday, June 5, 2023 3:41:21 PM To: sandialabs/pyttb ***@***.***> Cc: Dunlavy, Daniel M ***@***.***>; State change ***@***.***> Subject: [EXTERNAL] Re: [sandialabs/pyttb] Class inheritance and docstrings (PR #131) In decreasing frequency: * In the code * python command line when I am trying something out help(sptensor) * readthedocs I just pulled your PR locally and ran pytest tests/ --cov=pyttb --cov-report=term-missing --packaging and pylint was upset for missing module doc strings on all these files. If we find them redundant or unnecessary we can disable that pylint rule. I am surprised you didn't hit this locally or in CI. Maybe my pylint is old and this has changed. https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/missing-module-docstring.html — Reply to this email directly, view it on GitHub<#131 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHIY2ISJHSRK63HQOXDEUGTXJZHADANCNFSM6AAAAAAYZVH5TE>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[ntjohnson1]

Yes we should probably investigate making our versioning a little stricter at some point. Note I opened this draft PR that basically reverted the object and module doc string changes then enforced the current state of our pylint #132 maybe lets continue discussing there. Note I acknowledge that coveralls works but it awkwardly shows some doc strings as not being covered which seems incorrect. This isn't high priority if we wanted to merge my change then figure out the coveralls integration as follow up