[] [SVCS-694] Fix wb_path imports #334

NyanHelsing · 2018-04-10T17:13:36Z

Ticket

https://openscience.atlassian.net/browse/SVCS-694

Purpose

Improve code consistency

Changes

wb_path is being imported qualified from wabterbutler.core.path. Most other files import the WaterButlerPath class directly. This change updates core/provider to align with other files.

Side effects

QA Notes

Deployment Notes

coveralls · 2018-04-10T18:44:54Z

Coverage decreased (-0.01%) to 89.851% when pulling be645cc on birdbrained:ft/wb-path-import-fix into 98b057c on CenterForOpenScience:develop.

cslzchen

Love this PR. Three minor changes as commented. 🎆

cslzchen · 2018-04-10T19:21:59Z

waterbutler/core/provider.py

 from waterbutler import settings as wb_settings
-from waterbutler.core.metrics import MetricsRecord
+from waterbutler.core import exceptions


Combine exceptions and streams

cslzchen · 2018-04-10T19:22:35Z

waterbutler/core/provider.py

 from waterbutler.core.utils import RequestHandlerContext
+from waterbutler.core.utils import ZipStreamGenerator


Combine ZipStreamGenerator and RequestHandlerContext.

cslzchen · 2018-04-10T19:26:04Z

waterbutler/core/provider.py

 from waterbutler import settings as wb_settings
-from waterbutler.core.metrics import MetricsRecord
+from waterbutler.core import exceptions
 from waterbutler.core import metadata as wb_metadata


from waterbutler.core.metadata import (..., ..., ..., )

Remove some of the qualified imports as they are not needed to be qualified. Make some of the import more explicit as to which symbols are being imported.

cslzchen

Fix import styles and usages
Fix the first line for DocStr
Fix type annotations
In the end, use invoke mypy to fix further typing errors

cslzchen · 2018-04-18T18:11:20Z

waterbutler/core/provider.py

 from waterbutler.core import exceptions
-from waterbutler.core import path as wb_path
-from waterbutler import settings as wb_settings
+from waterbutler.core.metadata import (


Let's use the following style (several occurrences):

from waterbutler.core.metadata import (BaseMetadata, BaseFileMetadata, BaseFolderMetadata, )

Please use the WB's style (several occurrences).

cslzchen · 2018-04-18T18:12:32Z

waterbutler/core/provider.py

+        rename: str=None,
+        conflict: str='replace',
+        handle_naming: bool=True
+    ) -> typing.Tuple[wb_metadata.BaseMetadata, bool]:


Update the typing: wb_metadata.BaseMetadata -> BaseMetadata. (Several occurrences)

In addition, using

from typing import ...

will reduce line space and make the type annotation look less ugly.

cslzchen · 2018-04-18T18:28:15Z

waterbutler/core/provider.py

+        conflict: str='replace',
+        handle_naming: bool=True
+    ) -> typing.Tuple[wb_metadata.BaseMetadata, bool]:
+        """


First sentence of DocStr starts on the same line of """. (Several occurrences)

cslzchen · 2018-04-18T18:37:53Z

waterbutler/core/provider.py

-        """Moves a file or folder from the current provider to the specified one
+    async def move(
+        self,
+        dest_provider: 'BaseProvider',


Import the BaseProvider and use it as it is instead of a string (similar to BaseMetadata, etc.)

cslzchen · 2018-04-18T18:38:34Z

waterbutler/core/provider.py

        return parent_path.child(meta_data.name, _id=meta_data.path.strip('/'),
                                 folder=meta_data.is_folder)

-    async def revisions(self, path: wb_path.WaterButlerPath, **kwargs):
-        """Return a list of :class:`.BaseFileRevisionMetadata` objects representing the revisions
+    async def revisions(self, path: WaterButlerPath, **kwargs): -> List[BaseFileRevisionMetadata]


: goes after return type.

cslzchen · 2018-04-18T18:40:46Z

waterbutler/core/provider.py

-            for item in items[i:i + wb_settings.OP_CONCURRENCY]:  # type: ignore
-                futures.append(asyncio.ensure_future(
+            for item in items[i:i + OP_CONCURRENCY]:  # type: ignore
+                future = asyncio.ensure_future(


This is a functional change that I will test locally. Need to verify that the current future is not used in the for loop.

cslzchen · 2018-04-18T18:47:52Z

waterbutler/core/provider.py

+    ResponseStreamRenderer,
+    ZipStreamRenderer
+)
+from waterbutler.core.utils import (


Put them in one line if the length is less than 100. I assume this is the case here.

… into ft/wb-path-import-fix

cslzchen · 2018-05-07T21:12:33Z

Reviewing this now but meanwhile please remove this merge commit be645cc and use rebase, thanks. @birdbrained

cslzchen

Major Issue - Import Style

I understand that there are different preferences for line breaking and indentation for import (and many other styles as well). I am not going to argue which one is better but please use the following one for consistence:

from waterbutler.core.metadata import (BaseMetadata,
                                       BaseFileMetadata,
                                       BaseFolderMetadata, )

The first time I saw this, I felt similarly that it looked very different from what I had been used to. However, I started using this style anyway. The main reason I switched to cater for what WB/MFR already has is consistence.

We already have a style to follow, which was decided about half a year ago and we have been applying them gradually instead of a full refactor. Changing them again to a new style (maybe better, maybe not) doesn't make sense.

CC @felliott

cslzchen · 2018-05-07T21:13:48Z

waterbutler/core/provider.py

@@ -1,24 +1,41 @@
 import abc
 import time
-import typing
+from typing import (


If it fits within one line, don't break it.

from typing import List, Set, Tuple, Callable, Union

cslzchen · 2018-05-07T21:16:04Z

waterbutler/core/provider.py

 from waterbutler.core import exceptions
-from waterbutler.core import path as wb_path
-from waterbutler import settings as wb_settings
+from waterbutler.core.metadata import (


Please use the WB's style (several occurrences).

cslzchen · 2018-05-07T21:26:00Z

waterbutler/providers/googledrive/provider.py

@@ -647,7 +647,7 @@ def _build_upload_metadata(self, folder_id: str, name: str) -> dict:
        ) as resp:
            try:
                data = await resp.json()
-            except:  # some 404s return a string instead of json
+            except Exception:  # some 404s return a string instead of json


It is a good first step by using Exception. How about one step further? Take a look at the aiohttp library and you will find the specific exceptions to use here.

cslzchen requested changes Apr 10, 2018

View reviewed changes

Remove unneccesarily qualified imports.

d50d4b8

Remove some of the qualified imports as they are not needed to be qualified. Make some of the import more explicit as to which symbols are being imported.

NyanHelsing force-pushed the ft/wb-path-import-fix branch from 9fd0534 to d50d4b8 Compare April 10, 2018 20:48

NyanHelsing added 3 commits April 10, 2018 17:06

updates

6aa3f09

Fix line lengths of function sigs

81ef6e6

Make future loop more readable

6e612b4

NyanHelsing changed the title ~~[] [] Fix wb_path imports~~ [] [SVCS-694] Fix wb_path imports Apr 18, 2018

cslzchen requested changes Apr 18, 2018

View reviewed changes

cslzchen added the Add'l Dev label Apr 18, 2018

NyanHelsing added 2 commits May 1, 2018 14:44

Fix linter

37a7124

Merge branch 'develop' of github.com:CenterForOpenScience/waterbutler…

be645cc

… into ft/wb-path-import-fix

cslzchen requested changes May 7, 2018

View reviewed changes

cslzchen added On Hold Do Not Merge and removed Add'l Dev labels Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[] [SVCS-694] Fix wb_path imports #334

[] [SVCS-694] Fix wb_path imports #334

NyanHelsing commented Apr 10, 2018 •

edited by cslzchen

Loading

coveralls commented Apr 10, 2018 •

edited

Loading

cslzchen left a comment

cslzchen Apr 10, 2018

cslzchen Apr 10, 2018

cslzchen Apr 10, 2018

cslzchen left a comment

cslzchen Apr 18, 2018

cslzchen May 7, 2018

cslzchen Apr 18, 2018

cslzchen Apr 18, 2018

cslzchen Apr 18, 2018

cslzchen Apr 18, 2018

cslzchen Apr 18, 2018

cslzchen Apr 18, 2018

cslzchen Apr 18, 2018

cslzchen commented May 7, 2018

cslzchen left a comment •

edited

Loading

cslzchen May 7, 2018

cslzchen May 7, 2018

cslzchen May 7, 2018

		from waterbutler.core.utils import RequestHandlerContext
		from waterbutler.core.utils import ZipStreamGenerator

[] [SVCS-694] Fix wb_path imports #334

Are you sure you want to change the base?

[] [SVCS-694] Fix wb_path imports #334

Conversation

NyanHelsing commented Apr 10, 2018 • edited by cslzchen Loading

Ticket

Purpose

Changes

Side effects

QA Notes

Deployment Notes

coveralls commented Apr 10, 2018 • edited Loading

cslzchen 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

cslzchen 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

cslzchen commented May 7, 2018

cslzchen left a comment • 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

NyanHelsing commented Apr 10, 2018 •

edited by cslzchen

Loading

coveralls commented Apr 10, 2018 •

edited

Loading

cslzchen left a comment •

edited

Loading