Refactor code #53

VitaliyaIoffe · 2021-12-13T19:13:06Z

isort
flake8 refactoring
typehinting

s3repo/model.py

VitaliyaIoffe · 2021-12-13T19:21:21Z

app.py

-    env_common_settings = {}
-    env_common_settings['credentials'] = \
-        json.loads(os.environ.get('RWS_CREDENTIALS'))
+    if (cred := os.environ.get('RWS_CREDENTIALS')) is None:


be careful: this assignment is allowed from python 3.8

Not related with the commit.
given your comment, I would prefer it to stay the way it was.

I don't think so due to in case of staying with your way I should describe type manually. It is a wrong way I guess, otherwise, the type would be assigned dynamically.

s3repo/model.py

VitaliyaIoffe · 2021-12-13T19:23:16Z

s3repo/view.py


        return render_template('index.html', **list_parameters)

    @staticmethod
-    def _get_file(path, response):
+    def _get_file(path, response) -> Response:


path and response are not described

path - str
response - described in https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.get_object as I understand.

s3repo/controller.py

LeonidVas

Hi! Thank you for the patchset.
Common comments:

isort: sort imports -> lint: add sorting of imports with isort
I think this commit is pointless without adding a workflow to CI/CD. But really useful with adding to CI/CD.
flake8: fix style -> codestyle: fix style according flake8. The same point as in the previous one.
typing: add typing -> codestyle: add typing. I think it's enough to specify the types for the arguments and return values of the functions. Specify types for internal variables - overkill.

app.py

LeonidVas · 2021-12-15T16:00:51Z

s3repo/package.py

@@ -1,5 +1,6 @@
 """Description of the uploaded / downloaded package."""

+


Not related with the commit.

upd: have to fix

app.py

LeonidVas · 2021-12-15T20:17:12Z

app.py

-    env_model_settings['secret_access_key'] = os.getenv('S3_SECRET_KEY')
-    env_model_settings['public_read'] = get_bool_env('S3_PUBLIC_READ', False)
-    env_model_settings['force_sync'] = get_bool_env('RWS_FORCE_SYNC', False)
+    env_model_settings = {


Not related with the commit.

I don't agree, see comments above

s3repo/view.py

LeonidVas · 2021-12-15T20:37:57Z

s3repo/view.py

@@ -27,42 +28,43 @@ def _readable_size(size):
                return readable_size
            size /= 1024.0

-        return "%3.1f B" % (size)
+        return "%3.1f B" % size


Not related with the commit.
What's the code style rule being violated here?

Remove redundant parentheses

LeonidVas · 2021-12-15T20:38:28Z

s3repo/view.py

-                           'path': path,
-                           'parent_path': parent_path,
-                           'items': readable_items}
+        list_parameters = {


Not related with the commit.

LeonidVas · 2021-12-15T20:41:51Z

s3repo/view.py


        return render_template('index.html', **list_parameters)

    @staticmethod
-    def _get_file(path, response):
+    def _get_file(path, response) -> Response:


path - str
response - described in https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.get_object as I understand.

LeonidVas · 2021-12-15T20:42:20Z

s3repo/view.py

-        """Show a directory or download a file according to the
-        "subpath" path.
+    def dispatch_request(self, subpath='/') -> Response:
+        """Show a directory or download a file according to the "subpath" path.


Not related with the commit.
What's the code style rule being violated here?

filonenko-mikhail · 2022-01-19T06:29:01Z

Is such pull request still actual?

LeonidVas · 2022-01-20T06:58:45Z

Is such pull request still actual?

Yep. The author planned to finish it.

VitaliyaIoffe · 2022-01-31T17:01:26Z

Is such pull request still actual?

Yep. The author planned to finish it.

The author is here again!

But I am not a team member and can't push to the pr :( . What way should I use to finish? Use new pr from my repo?
@LeonidVas, please direct me in the messenger, I don't check any updates now from GitHub.

Totktonada · 2022-01-31T19:04:21Z

I temporary give you write access to finish this PR (sorry, I can't keep this permission forever).

VitaliyaIoffe · 2022-01-31T19:07:38Z

I temporary give you write access to finish this PR (sorry, I can't keep this permission forever).

Not a problem. Thank you!

VitaliyaIoffe · 2022-02-05T17:16:44Z

Just rebased. It takes more time to finish.

VitaliyaIoffe · 2022-04-08T13:51:50Z

@LeonidVas

LeonidVas · 2023-02-13T16:28:22Z

Hi! Sorry I didn't review this patch in time. We are planning to migrate to nexus and archive this service, so no work is being done here other than bug fixes.
If company policy is changed, I will reopen PR, update and take it on board if Yaroslav doesn't mind.

VitaliyaIoffe requested a review from LeonidVas December 13, 2021 19:13

VitaliyaIoffe linked an issue Dec 13, 2021 that may be closed by this pull request

Add typehinting #37

Open

VitaliyaIoffe commented Dec 13, 2021

View reviewed changes

s3repo/model.py Outdated Show resolved Hide resolved

VitaliyaIoffe commented Dec 13, 2021

View reviewed changes

s3repo/model.py Outdated Show resolved Hide resolved

VitaliyaIoffe commented Dec 13, 2021

View reviewed changes

s3repo/controller.py Show resolved Hide resolved

LeonidVas requested changes Dec 15, 2021

View reviewed changes

VitaliyaIoffe force-pushed the VitaliyaIoffe/37-add-typehinting branch from 07e83bc to fff4f87 Compare February 5, 2022 17:16

VitaliyaIoffe force-pushed the VitaliyaIoffe/37-add-typehinting branch 2 times, most recently from a3523bd to 946e3d5 Compare February 9, 2022 22:40

VitaliyaIoffe added 2 commits February 22, 2022 23:28

isort: sort imports

2b8e3e9

codestyle: fix style according flake8

41f8a48

VitaliyaIoffe force-pushed the VitaliyaIoffe/37-add-typehinting branch 2 times, most recently from 49675fc to 56269a5 Compare February 22, 2022 20:37

codestyle: add typing

6343809

VitaliyaIoffe force-pushed the VitaliyaIoffe/37-add-typehinting branch from 56269a5 to 6343809 Compare February 22, 2022 20:39

VitaliyaIoffe requested a review from LeonidVas February 22, 2022 20:40

LeonidVas closed this Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor code #53

Refactor code #53

VitaliyaIoffe commented Dec 13, 2021 •

edited

Loading

VitaliyaIoffe Dec 13, 2021

LeonidVas Dec 15, 2021

VitaliyaIoffe Feb 5, 2022

VitaliyaIoffe Dec 13, 2021

LeonidVas Dec 15, 2021

LeonidVas left a comment •

edited

Loading

LeonidVas Dec 15, 2021

VitaliyaIoffe Feb 9, 2022 •

edited

Loading

LeonidVas Dec 15, 2021

VitaliyaIoffe Feb 9, 2022

LeonidVas Dec 15, 2021

VitaliyaIoffe Feb 22, 2022

LeonidVas Dec 15, 2021

LeonidVas Dec 15, 2021

LeonidVas Dec 15, 2021

filonenko-mikhail commented Jan 19, 2022

LeonidVas commented Jan 20, 2022

VitaliyaIoffe commented Jan 31, 2022 •

edited

Loading

Totktonada commented Jan 31, 2022

VitaliyaIoffe commented Jan 31, 2022

VitaliyaIoffe commented Feb 5, 2022

VitaliyaIoffe commented Apr 8, 2022

LeonidVas commented Feb 13, 2023

		@@ -1,5 +1,6 @@
		"""Description of the uploaded / downloaded package."""

Refactor code #53

Refactor code #53

Conversation

VitaliyaIoffe commented Dec 13, 2021 • 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

LeonidVas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VitaliyaIoffe Feb 9, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filonenko-mikhail commented Jan 19, 2022

LeonidVas commented Jan 20, 2022

VitaliyaIoffe commented Jan 31, 2022 • edited Loading

Totktonada commented Jan 31, 2022

VitaliyaIoffe commented Jan 31, 2022

VitaliyaIoffe commented Feb 5, 2022

VitaliyaIoffe commented Apr 8, 2022

LeonidVas commented Feb 13, 2023

VitaliyaIoffe commented Dec 13, 2021 •

edited

Loading

LeonidVas left a comment •

edited

Loading

VitaliyaIoffe Feb 9, 2022 •

edited

Loading

VitaliyaIoffe commented Jan 31, 2022 •

edited

Loading