Update filesystem datastore to async/await #13

ntninja · 2020-01-08T22:23:22Z

Update filesystem datastore to async/await
Slightly restructure the datastore API again, everything important should now be exposed and datastore.core.* only needed by internal stuff

…should now be exposed and `datastore.core.*` only needed by internal stuff

AliabbasMerchant · 2020-01-28T08:26:41Z

datastore/core/test/conftest.py

-				assert len(sn) == length
-		except TypeError:
-			pass
+		for sn in self.stores:


AliabbasMerchant · 2020-01-28T08:30:38Z

datastore/__init__.py

+__version__ = "0.3.6"
+__author__ = "Juan Batiz-Benet, Alexander Schlarb"
+__email__ = "juan@benet.ai, alexander@ninetailed.ninja"
+__all__ = (


Nice work exposing those Classes directly.

AliabbasMerchant · 2020-01-28T09:20:59Z

datastore/__init__.py

+
+from .core.objectstore import NullDatastore as ObjectNullDatastore
+from .core.objectstore import DictDatastore as ObjectDictDatastore
+


We will also have to expose FileSystemDatastore and FileReader

My thought was that the filesystem datastore has to be explicitly imported using datastore.filesystem as it is not needed if another datastore (like leveldb or the in-memory dict one) is used.

FileReader is just an implementation detail of how the datastore manages to return its datastore.abc.ReceiveStream. Its not really meant to be used directly.

AliabbasMerchant · 2020-01-28T10:55:22Z

datastore/filesystem/filesystem.py

+		else:
+			buf = await self._file.read(DEFAULT_BUFFER_SIZE)
+
+		if len(buf) == 0:


Is it necessary to call .aclose() here? Won't the user explicitly call it?
And if we are already calling .aclose(), when the user explicitly calls .aclose(), wont there be some kind of error, saying that it is already closed?

Calling .aclose() here ensures that the file automatically be closed in an

async for chunk in await datastore.get("whatever"): …

kind of scenario. Otherwise the calling code would have to look like this:

async with await datastore.get("whatever") as file: async for chunk in file: …

… or the file descriptor would leak.

Calling aclose several times is not an error:

If the resource is already closed, then this method should silently succeed.

AliabbasMerchant · 2020-01-28T11:27:42Z

datastore/filesystem/filesystem.py

+		# Try to remove parent directories if they are empty
+		try:
+			parent = path.parent
+			while str(parent).startswith(self.root_path + os.path.sep) \


I didn't understand the logic here, so no comments

It tries to rmdir parent directories as long as

that directory is a subdirectory of the root_path

that is: the directory path starts with the string <…/some/root/path>/

and the directory's parent is not the directory itself again

this guards against the case of the root directory being the current directory (.) whose .parent property is also . (I'm am not sure anymore if this isn't subsumed the first rule however as ".".startswith("./") is False either way…?

I probably should a comment to the code about this, shouldn't I?

AliabbasMerchant · 2020-01-28T11:30:57Z

@Alexander255 Sorry for the delay in reviewing.

I went through all the files. They look good to me.
(3 out of 56 code tests are failing. [TestQuery.test_cursor, TestOrder.test_basic, TestFilter.test_basic] And these are from broken modules, right?
Also, can you please try fixing Travis?)

Some of the code snippets in the doc comments have incorrect imports. But I think we can fix them later on.

Can we, in a future commit, shift all the test files to a separate folder outside?

Please correct me if I am wrong:

You are exposing Abstract Classes via datastore.abc
And exposing the utils via datastore.utils
mypy is a dependency because of the typing and all.

adamjsawicki · 2020-01-28T16:54:32Z

@AliabbasMerchant I fixed travis problems in ntninja#1. @Alexander255 made changes you'd suggested.

ntninja · 2020-01-28T17:08:14Z

@AliabbasMerchant: Thank you for your review, better late than never I guess! 🙂

I went through all the files. They look good to me.
(3 out of 56 code tests are failing. [TestQuery.test_cursor, TestOrder.test_basic, TestFilter.test_basic] And these are from broken modules, right?

I think so, but they should be skipped in the test suite. I created #14 to document what I think query will be in the future. After doing that there should be no more failing tests. Note that that work is not py-ipfs related, it's just something to make this library complete again.

Also, can you please try fixing Travis?)

@swixx did that but it's waiting for this to be merged.

Some of the code snippets in the doc comments have incorrect imports. But I think we can fix them later on.

Created #16 about this.

Can we, in a future commit, shift all the test files to a separate folder outside?

Sure, now that the tests (almost) only use the public API, I seeno reason to keep them in those folders.

Please correct me if I am wrong:

You are exposing Abstract Classes via datastore.abc

And exposing the utils via datastore.utils

Yes to both.

mypy is a dependency because of the typing and all.

No, mypy is only a dependency for type checking. It is not used during testing or in the final package otherwise. (However the stdlib typing module is used a lot for proper typing declarations in all sorts of places, making it a hard dependency.)

If you are referring to this change. This is about forcing mypy to be downloaded in source form, so that mypy_run.py can monkey patch the use of some extra typing definitions into the mypy validator. (The previous form was incorrect and caused issues.)

AliabbasMerchant · 2020-01-28T17:47:14Z

Great!

If you are referring to this change. This is about forcing mypy to be downloaded in source form, so that mypy_run.py can monkey patch the use of some extra typing definitions into the mypy validator. (The previous form was incorrect and caused issues.)

Yes, I was referring to that change. Got it 👍

Lets merge this!

Then we can merge the Travis part that @swixx did, and #16
And then query and shifting of tests to a separate folder.

ntninja · 2020-01-28T18:09:58Z

@AliabbasMerchant: Ok, I'll merge this once I added that comment to the filesystem.py source file. (Also I realized that the rmdir should be optional entirely.)

Probably later this evening or tomorrow.

AliabbasMerchant · 2020-01-28T18:20:05Z

Okay.
Also, @Alexander255, can you please tell the ipfs admins, to include this repo under the python team? (I am not aware of the procedure to be followed to request this.)

…tories should be used along with an explanation of what actually happens when this is done

AliabbasMerchant · 2020-01-29T05:42:43Z

datastore/filesystem/filesystem.py

 		try:
 			parent = path.parent
-			while str(parent).startswith(self.root_path + os.path.sep) \
+			# Attempt to remove all parent directories as long as the
+			# parent directory is:


Got it! Thanks!
It was a wise decision to use pathlib 👍

ntninja added 3 commits January 8, 2020 23:20

Update filesystem datastore to async/await

f5cc6c2

Slightly restructure the datastore API again, everything important …

0c2cbb3

…should now be exposed and `datastore.core.*` only needed by internal stuff

Fix up mypy results again

e80a1d7

ntninja mentioned this pull request Jan 27, 2020

Make this library async-friendly #8

Closed

adamjsawicki mentioned this pull request Jan 28, 2020

Fix styleck errors ntninja/py-datastore#1

Merged

AliabbasMerchant reviewed Jan 28, 2020

View reviewed changes

datastore/core/test/conftest.py

assert len(sn) == length

except TypeError:

pass

for sn in self.stores:

Copy link

Member

AliabbasMerchant Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

AliabbasMerchant reviewed Jan 28, 2020

View reviewed changes

AliabbasMerchant approved these changes Jan 28, 2020

View reviewed changes

This was referenced Jan 28, 2020

Update README.md to current status and conform to Standard README format #15

Merged

What are the datastores in roadmap. #10

Open

ntninja mentioned this pull request Jan 28, 2020

Fix up the doctests #16

Open

Add extra type hints and a parameter to configure whether empty direc…

5cb05be

…tories should be used along with an explanation of what actually happens when this is done

ntninja merged commit 4798c7a into ipfs-shipyard:master Jan 28, 2020

AliabbasMerchant reviewed Jan 29, 2020

View reviewed changes

adamjsawicki mentioned this pull request Feb 2, 2020

Move tests to external folder #21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update filesystem datastore to async/await #13

Update filesystem datastore to async/await #13

ntninja commented Jan 8, 2020

AliabbasMerchant Jan 28, 2020

AliabbasMerchant Jan 28, 2020

AliabbasMerchant Jan 28, 2020

ntninja Jan 28, 2020

AliabbasMerchant Jan 29, 2020

AliabbasMerchant Jan 28, 2020

ntninja Jan 28, 2020

AliabbasMerchant Jan 28, 2020

ntninja Jan 28, 2020

AliabbasMerchant commented Jan 28, 2020

adamjsawicki commented Jan 28, 2020

ntninja commented Jan 28, 2020

AliabbasMerchant commented Jan 28, 2020

ntninja commented Jan 28, 2020

AliabbasMerchant commented Jan 28, 2020

AliabbasMerchant Jan 29, 2020


		from .core.objectstore import NullDatastore as ObjectNullDatastore
		from .core.objectstore import DictDatastore as ObjectDictDatastore

Update filesystem datastore to async/await #13

Update filesystem datastore to async/await #13

Conversation

ntninja commented Jan 8, 2020

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

AliabbasMerchant commented Jan 28, 2020

adamjsawicki commented Jan 28, 2020

ntninja commented Jan 28, 2020

AliabbasMerchant commented Jan 28, 2020

ntninja commented Jan 28, 2020

AliabbasMerchant commented Jan 28, 2020

Choose a reason for hiding this comment