[SYNPY-1370] Documentation Upgrade

[jaymedina]

This PR will update the following documentation:

• Small few left: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/annotations.py
• Evaluation - The section at the top of the file should be changed from RST into MD format. See Table for an example: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/evaluation.py
• Wiki The section at the top of the file should be changed from RST into MD format. See Table for an example:: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/wiki.py
• https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/core/retry.py
  Should also add this to https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/docs/reference/core.md
• https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/core/version_check.py
• https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/core/upload/multipart_upload.py
• https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseutils/sync.py

[pep8speaks]

Hello @jaymedina! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file synapseclient/annotations.py:

Line 314:89: E501 line too long (109 > 88 characters)
Line 324:89: E501 line too long (102 > 88 characters)
Line 325:89: E501 line too long (98 > 88 characters)
Line 363:89: E501 line too long (119 > 88 characters)

• In the file synapseclient/core/retry.py:

Line 71:89: E501 line too long (101 > 88 characters)
Line 72:89: E501 line too long (94 > 88 characters)
Line 73:89: E501 line too long (100 > 88 characters)
Line 74:89: E501 line too long (97 > 88 characters)
Line 75:89: E501 line too long (109 > 88 characters)
Line 78:89: E501 line too long (89 > 88 characters)
Line 186:89: E501 line too long (116 > 88 characters)

• In the file synapseclient/core/upload/multipart_upload.py:

Line 2:89: E501 line too long (155 > 88 characters)
Line 3:89: E501 line too long (119 > 88 characters)
Line 66:89: E501 line too long (99 > 88 characters)
Line 604:89: E501 line too long (140 > 88 characters)
Line 605:89: E501 line too long (98 > 88 characters)
Line 609:89: E501 line too long (101 > 88 characters)
Line 612:89: E501 line too long (104 > 88 characters)
Line 615:89: E501 line too long (106 > 88 characters)
Line 675:89: E501 line too long (90 > 88 characters)
Line 676:89: E501 line too long (96 > 88 characters)
Line 678:89: E501 line too long (102 > 88 characters)

• In the file synapseclient/core/version_check.py:

Line 39:89: E501 line too long (91 > 88 characters)
Line 102:89: E501 line too long (94 > 88 characters)
Line 133:89: E501 line too long (101 > 88 characters)

• In the file synapseclient/evaluation.py:

Line 5:89: E501 line too long (99 > 88 characters)
Line 19:89: E501 line too long (107 > 88 characters)
Line 24:89: E501 line too long (111 > 88 characters)
Line 29:89: E501 line too long (107 > 88 characters)
Line 40:89: E501 line too long (112 > 88 characters)
Line 84:89: E501 line too long (115 > 88 characters)
Line 87:89: E501 line too long (122 > 88 characters)
Line 102:89: E501 line too long (97 > 88 characters)
Line 223:89: E501 line too long (91 > 88 characters)
Line 262:89: E501 line too long (96 > 88 characters)

• In the file synapseclient/wiki.py:

Line 86:89: E501 line too long (112 > 88 characters)

• In the file synapseutils/sync.py:

Line 267:89: E501 line too long (92 > 88 characters)
Line 648:89: E501 line too long (99 > 88 characters)
Line 672:89: E501 line too long (91 > 88 characters)
Line 824:89: E501 line too long (90 > 88 characters)
Line 1156:89: E501 line too long (97 > 88 characters)

Comment last updated at 2024-01-03 16:38:02 UTC

[BryanFauble]

Excellent start! When you get to another stopping point please use the Re-request Review icon next to my name in the Reviewers section and I will take a look.

[jaymedina]

I removed the explicit call to ::: synapseclient.core.upload.multipart_upload._multipart_upload because it looked confusing in the documentation, and I wanted to have a conversation about it later. From a quick scan of the documentation, this looks to be the only hidden function that's explicitly documented. I see why that is: The returns of the other functions are calls to _multipart_upload, and so references to it are made in the Multipart section of Core. But this goes back to a previous conversation of whether or not we should expose the documentation of a hidden function and vice versa. To me if something is hidden with _, it's best not to add it to the documentation because it will be hidden from the user unless it's explicitly called, so it's best to avoid possible confusion with users that aren't experts in Python. But I'm up for iterating on this before a final review.

[thomasyu888]

One thing we can explicitly state is that people should use functions within core at their own risk. There's not really anything "hidden" within Python packages, but the functions within core are for developers only and aren't part of the "public" Python API.

[jaymedina]

What I mean by hidden is that the functions led by _ are denoted to be for internal use only, and won't be available unless explicitly called, so it's not functionality that's immediately available to users (a user would have to look inside the code to know it's there).

But based on what you're saying, it sounds like we want to document everything in core, both internal and external functions since core is for users that are more advanced to begin with. If that's the case, I would re-add _multipart_upload and update its docstring, and also expose retry.py's _get_message

[thomasyu888]

All functions in core and this package should have docstrings even if they aren't surfaced on the online python documentation site for development purposes and code readability.

I don't think we actually need all of core to be available in the online python documentation site, but we should document that the functions in core are subject to change at any time and for python client users to use at their own risk.

[jaymedina]

Hey @BryanFauble , happy new year 🙂 Just wanted quick clarification on the pep8-speaks status report on my PR: Did you want us to follow black's 88 character line-length rule you configured pep8-speaks to follow? Or should I just focus on resolving the SonarCloud issues (SonarCloud is following 120 char. limit)?

[BryanFauble]

Hey @BryanFauble , happy new year 🙂 Just wanted quick clarification on the pep8-speaks status report on my PR: Did you want us to follow black's 88 character line-length rule you configured pep8-speaks to follow? Or should I just focus on resolving the SonarCloud issues (SonarCloud is following 120 char. limit)?

@jaymedina Let's follow the 88 character limit. Sometimes we won't be able to always follow it. In some cases I have seen that having a URL that spans multiple lines breaks when it's rendered on the doc site, but we should strive to follow it as close as we can.

I went ahead and updated the SonarCloud rule to follow this 88 limit.

[jaymedina]

@jaymedina The other option is to mark it as deprecated. Point them to a replacement if we have it or that it'll be removed in the future. See my comment here: #1026 (comment)

@BryanFauble, there are no replacements to these so I won't mark them as deprecated, and they're called in other parts of the repo (client.py). I think I'll table this for now until the Python client refactoring happens, as I think it's just a design preference at this point. This PR is ready for review now. I allowed for a little bit of scope creep when it felt appropriate and improved the overall documentation (added docstrings to methods/functions without them, updated docstrings of internal functions). Per our conversation at the sprint planning yesterday I'm going to ignore the SonarCloud code smell messages related to lines beyond 88 chars. I can make a low priority Jira ticket to tackle formatting at a later time. Let me know your thoughts. Thanks!

Edit: Currently investigating, but not sure what's going on with the Ubuntu 3.8 tests. I'm locally running the failing unit tests (tests/unit/synapseutils/unit_test_synapseutils_sync.py) and they're all passing on my Mac machine. The failures seem to have started in 12ce445, although I wonder if we would have also seen it as early as 6aa4bb4 had it not been for the pre-commit fail.

[BryanFauble]

Excellent works! Thanks for taking your time to think through all of these changes.

[sonarqubecloud]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

41 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[jaymedina]

Great, thanks reviewers for the help