Add type hints and improve or add missing docstrings #80

barthisrael · 2023-08-09T21:17:47Z

This PR applies the following changes:

Add type hints to the code;
Improve existing docstrings of methods and functions;
Add missing docstrings to modules, methods, functions and classes.

References: BAR-89.

barthisrael · 2023-08-09T21:21:29Z

Besides what I already submitted in this PR, I thought of applying a couple other changes too:

Rename write_jobs_files as create_job_file
Make create_output_file create the output dir instead of write_jobs_files doing that.

Thoughts?

martinmarques

Please move the documentation improvements to a separate PR for better review handling (put me as a reviewer on that docs PR).
There are two changes requested. One is related to breaking a line which I personally don't feel it's that long that would require a line break. I saw similar line breaks in other parts of the code, which also didn't feel appropriate given the original length.
I left the actual code review work for Mike and Mario

martinmarques · 2023-08-10T07:03:37Z

pg_backup_api/pg_backup_api/run.py

@@ -94,32 +158,55 @@ def recovery_operation(args):
    end_time = server_ops.time_event_now()
    content["success"] = success
    content["end_time"] = end_time
-    content["output"] = output.decode() if isinstance(output, bytes) else output
+    content["output"] = output.decode() if isinstance(output, bytes) \


I don't see any advantages to splitting the line here. It's not like the original line is terribly long that would make sense to split it.

I only did that to silence flake8.
I'm not sure if we intend to add a linter to the project later to check the code on pushed commits?
I can revert that change and silence the flake8 warning if you prefer.

We could add black/flake8 to our workflow if you like. I usually try to prioritise readability than form. The above splitting I think proves my point.

We can convert that into 2 lines or write a method to sanitise content{"output"] so we can silence flake8 and will be easy to read IMO.

Sounds good. I'm reverting the change for now. Maybe we can even set flake8 to allow max-line length as 120 for example, like we have in Patroni.

martinmarques · 2023-08-10T07:08:22Z

pg_backup_api/pg_backup_api/server_operation.py

        if not self.operation_id:
            raise Exception("operation_id is required here")

-        file_name = "{}.json".format(self.operation_id)
-        fpath = join(file_type, file_name)
+        file_name = f"{self.operation_id}.json".


Are you missing something after the trailing dot?
Said differently, is that dot a leftover from the original code?

I guess I added that by mistake after running flake8, otherwise it would have showed something like this when I ran the linter:

pg_backup_api/server_operation.py:228:50: E999 SyntaxError: invalid syntax

barthisrael · 2023-08-10T11:16:46Z

I converted this PR into a draft. I'll work on splitting this into different PRs, each one covering a different type of change.

barthisrael · 2023-08-10T11:56:38Z

Reverted the changes on README.md

barthisrael · 2023-08-10T13:42:06Z

This PR now contains only changes related to type hints and docstrings

mikewallace1979

Thanks for adding the docstrings - looks great overall.

I suggested a handful of minor/superficial changes to the wording and also found an issue with the conditional import causing errors when TYPE_CHECKING is False.

mikewallace1979 · 2023-08-11T08:42:20Z

pg_backup_api/pg_backup_api/app.py

+"""
+Used when running pg-backup-api REST API server as an WSGI application.
+
+Load Barman configuration, set up logging for WSGI, and set up an JSON console


Should be a JSON not an JSON.

mikewallace1979 · 2023-08-11T08:42:32Z

pg_backup_api/pg_backup_api/app.py

+Used when running pg-backup-api REST API server as an WSGI application.
+
+Load Barman configuration, set up logging for WSGI, and set up an JSON console
+output writter.


writer instead of writter

mikewallace1979 · 2023-08-11T08:49:56Z

pg_backup_api/pg_backup_api/logic/utility_controller.py

@@ -31,9 +33,19 @@
 from pg_backup_api.run import app
 from pg_backup_api.server_operation import ServerOperation, ServerOperationConfigError

+if TYPE_CHECKING:  # pramga: no cover


pramga -> pragma

mikewallace1979 · 2023-08-11T08:52:46Z

pg_backup_api/pg_backup_api/logic/utility_controller.py

-def diagnose():
+def diagnose() -> Response:
+    """
+    ``Handle GET`` request to ``/diagnose``.


Looks like Handle shouldn't be in the double backticks here.

mikewallace1979 · 2023-08-11T08:58:05Z

pg_backup_api/pg_backup_api/logic/utility_controller.py

+        operation.
+    :param operation_id: ID of the recovery operation previously created
+        through pg-backup-api.
+    :return: if *server_name* and *operation_id* are valid, return an JSON


mikewallace1979 · 2023-08-11T09:34:00Z

pg_backup_api/pg_backup_api/utils.py

@@ -16,39 +16,59 @@
 # You should have received a copy of the GNU General Public License
 # along with Postgres Backup API.  If not, see <http://www.gnu.org/licenses/>.

+"""
+Utilitary functions and constants used through pg-backup-api code.


Utilitary -> Utility

mikewallace1979 · 2023-08-11T10:23:46Z

pg_backup_api/pg_backup_api/utils.py


 from flask import Flask

 import barman
 from barman import config
 from barman.infofile import BackupInfo

+if TYPE_CHECKING:  # pragma: no cover


On python 3.6 and 3.7 at least (I haven't tested other versions yet) the imports on the following lines are still required, even if TYPE_CHECKING is False. Currently it fails as follows:

$ ~/.local/bin/pg-backup-api Traceback (most recent call last): File "/var/lib/barman/.local/bin/pg-backup-api", line 11, in <module> load_entry_point('pg-backup-api', 'console_scripts', 'pg-backup-api')() File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 476, in load_entry_point return get_distribution(dist).load_entry_point(group, name) File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2700, in load_entry_point return ep.load() File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2318, in load return self.resolve() File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2324, in resolve module = __import__(self.module_name, fromlist=['__name__'], level=0) File "/pg-backup-api/pg_backup_api/pg_backup_api/__init__.py", line 20, in <module> import pg_backup_api.logic.utility_controller File "/pg-backup-api/pg_backup_api/pg_backup_api/logic/utility_controller.py", line 31, in <module> from pg_backup_api.utils import load_barman_config, get_server_by_name, parse_backup_id File "/pg-backup-api/pg_backup_api/pg_backup_api/utils.py", line 46, in <module> def create_app() -> flask.app.Flask: NameError: name 'flask' is not defined

Oh, I missed quoting these.

mikewallace1979 · 2023-08-11T10:26:57Z

pg_backup_api/pg_backup_api/logic/utility_controller.py

@@ -31,9 +33,19 @@
 from pg_backup_api.run import app
 from pg_backup_api.server_operation import ServerOperation, ServerOperationConfigError

+if TYPE_CHECKING:  # pramga: no cover


Conditionally importing Request and Response causes the following failure when TYPE_CHECKING is False:

$ ~/.local/bin/pg-backup-api serve Traceback (most recent call last): File "/var/lib/barman/.local/bin/pg-backup-api", line 11, in <module> load_entry_point('pg-backup-api', 'console_scripts', 'pg-backup-api')() File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 476, in load_entry_point return get_distribution(dist).load_entry_point(group, name) File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2700, in load_entry_point return ep.load() File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2318, in load return self.resolve() File "/usr/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2324, in resolve module = __import__(self.module_name, fromlist=['__name__'], level=0) File "/pg-backup-api/pg_backup_api/pg_backup_api/__init__.py", line 20, in <module> import pg_backup_api.logic.utility_controller File "/pg-backup-api/pg_backup_api/pg_backup_api/logic/utility_controller.py", line 41, in <module> def diagnose() -> Response: NameError: name 'Response' is not defined

Missing quoting here too.

mikewallace1979 · 2023-08-11T10:36:13Z

pg_backup_api/pg_backup_api/utils.py

+
+    :param server: server from which to get a backup.
+    :param backup_id: ID of the backup to be retrieved. It accepts a few
+        wildcards:


Could we say aliases instead of wildcards here?

mikewallace1979 · 2023-08-11T10:48:48Z

README.md

-in the dependency tree. Besides that, you also need the `libpq` C library in
-your system (`libpq-dev` package on Debian based, or `libpq-devel` package on
-RedHat based).
+tbd


Did you mean to revert the README.md changes here?

No, something went wrong with the rebase, sorry.
I'm sure I accept head although it created problems with README.md 🤔
I'll turn this into a draft and check what is going on.

Signed-off-by: Israel Barth Rubio <[email protected]>

…ler.py` Signed-off-by: Israel Barth Rubio <[email protected]>

Signed-off-by: Israel Barth Rubio <[email protected]>

``` pg_backup_api/__main__.py:87:1: W391 blank line at end of file ``` Signed-off-by: Israel Barth Rubio <[email protected]>

mikewallace1979

Looks good - I think these commits can all squash down into a single commit which adds the type hints and docstrings.

gonzalemario · 2023-08-11T16:12:23Z

I really wished we could have squashed down before merging.

barthisrael · 2023-08-11T19:22:55Z

squash

@gonzalemario I guess you are mixing the PRs. This one was actually squashed into b4a7d1f

The one that is showing more commits on main is #82, which you mentioned about using "rebase and merge" to avoid having a merge commit -- maybe we should have used "squash and merge" for that one too :)

gonzalemario · 2023-08-11T21:51:10Z

Yeah. I wasn't explicit enough about the rebase, sorry.

barthisrael requested review from gonzalemario and mikewallace1979 August 9, 2023 21:17

martinmarques requested changes Aug 10, 2023

View reviewed changes

barthisrael marked this pull request as draft August 10, 2023 11:11

barthisrael changed the title ~~Overall improvements -- type hints, docstrings, README.md changes, f-strings, fixing undefined variables~~ WIP - Add type hints and improve or add missing docstrings Aug 10, 2023

barthisrael marked this pull request as ready for review August 10, 2023 13:41

barthisrael changed the title ~~WIP - Add type hints and improve or add missing docstrings~~ Add type hints and improve or add missing docstrings Aug 10, 2023

barthisrael force-pushed the studying/israel branch from 8161979 to b0a77ca Compare August 11, 2023 10:45

mikewallace1979 requested changes Aug 11, 2023

View reviewed changes

barthisrael marked this pull request as draft August 11, 2023 10:51

barthisrael added 9 commits August 11, 2023 08:05

Add docstrings to pg_backup_api/logic/__init__.py

c003ed8

Signed-off-by: Israel Barth Rubio <[email protected]>

Add type hints and docstrings to `pg_backup_api/logic/utility_control…

a2b38d7

…ler.py` Signed-off-by: Israel Barth Rubio <[email protected]>

Add docstrings to pg_backup_api/__init__.py

a186e40

Signed-off-by: Israel Barth Rubio <[email protected]>

Add docstrings to pg_backup_api/app.py

1eb9d93

Signed-off-by: Israel Barth Rubio <[email protected]>

Add type hints and docstrings to pg_backup_api/__main__.py

6cc1619

Signed-off-by: Israel Barth Rubio <[email protected]>

Add type hints and docstrings to pg_backup_api/run.py

a43e1f7

Signed-off-by: Israel Barth Rubio <[email protected]>

Add type hints and docstrings to pg_backup_api/server_operation.py

0434092

Signed-off-by: Israel Barth Rubio <[email protected]>

Add type hints and docstrings to pg_backup_api/utils.py

443d572

Signed-off-by: Israel Barth Rubio <[email protected]>

Apply suggestions from code review

09190be

Signed-off-by: Israel Barth Rubio <[email protected]>

barthisrael force-pushed the studying/israel branch from b0a77ca to 09190be Compare August 11, 2023 11:53

Fix flake8 issue

1f6c326

``` pg_backup_api/__main__.py:87:1: W391 blank line at end of file ``` Signed-off-by: Israel Barth Rubio <[email protected]>

barthisrael requested a review from mikewallace1979 August 11, 2023 12:02

barthisrael marked this pull request as ready for review August 11, 2023 12:05

mikewallace1979 approved these changes Aug 11, 2023

View reviewed changes

barthisrael merged commit b4a7d1f into main Aug 11, 2023
2 checks passed

barthisrael deleted the studying/israel branch August 11, 2023 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints and improve or add missing docstrings #80

Add type hints and improve or add missing docstrings #80

barthisrael commented Aug 9, 2023 •

edited

Loading

barthisrael commented Aug 9, 2023

martinmarques left a comment

martinmarques Aug 10, 2023

barthisrael Aug 10, 2023

gonzalemario Aug 10, 2023

gonzalemario Aug 10, 2023

barthisrael Aug 10, 2023

martinmarques Aug 10, 2023

barthisrael Aug 10, 2023

barthisrael commented Aug 10, 2023

barthisrael commented Aug 10, 2023

barthisrael commented Aug 10, 2023

mikewallace1979 left a comment

mikewallace1979 Aug 11, 2023

mikewallace1979 Aug 11, 2023

mikewallace1979 Aug 11, 2023

mikewallace1979 Aug 11, 2023

mikewallace1979 Aug 11, 2023

mikewallace1979 Aug 11, 2023

mikewallace1979 Aug 11, 2023

barthisrael Aug 11, 2023

mikewallace1979 Aug 11, 2023

barthisrael Aug 11, 2023

mikewallace1979 Aug 11, 2023

mikewallace1979 Aug 11, 2023

barthisrael Aug 11, 2023

mikewallace1979 left a comment

gonzalemario commented Aug 11, 2023

barthisrael commented Aug 11, 2023 •

edited

Loading

gonzalemario commented Aug 11, 2023

Add type hints and improve or add missing docstrings #80

Add type hints and improve or add missing docstrings #80

Conversation

barthisrael commented Aug 9, 2023 • edited Loading

barthisrael commented Aug 9, 2023

martinmarques 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

Choose a reason for hiding this comment

barthisrael commented Aug 10, 2023

barthisrael commented Aug 10, 2023

barthisrael commented Aug 10, 2023

mikewallace1979 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

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

mikewallace1979 left a comment

Choose a reason for hiding this comment

gonzalemario commented Aug 11, 2023

barthisrael commented Aug 11, 2023 • edited Loading

gonzalemario commented Aug 11, 2023

barthisrael commented Aug 9, 2023 •

edited

Loading

barthisrael commented Aug 11, 2023 •

edited

Loading