Refactor: use `Literals` in type hints #1680

srini047 · 2023-06-28T20:03:43Z

Fixes: #1670

I have made sure to protocol with Literals and options being ['pickle', 'protobuf']

JoanFM · 2023-06-28T20:09:31Z

We need you to comply with our contributing guidelines and signoff the commits.

https://github.com/docarray/docarray/blob/main/CONTRIBUTING.md

srini047 · 2023-06-29T16:10:11Z

@JoanFM I have signed the commits. Could you have a look at it?

JoanFM · 2023-06-29T16:43:32Z

@JoanFM I have signed the commits. Could you have a look at it?

THE CI is complaining about that, as well as mypy, black and ruff

srini047 · 2023-06-29T16:45:55Z

@JoanFM I have signed the commits. Could you have a look at it?

THE CI is complaining about that, as well as mypy, black and ruff

Well how do I proceed with. Any leads appreciated.
Thanks in advance!

samsja · 2023-07-03T14:54:09Z

docarray/store/helpers.py

@@ -131,12 +132,27 @@ def close(self):


 def _to_binary_stream(
+    self, protocol: Literal['pickle', 'protobuf'],


default value should still be protobuf here and we should not change the order of the argument (protocol should stay at the same place as before)

I would want to make it clear if the protocol is in the right place and order (i.e. 2nd argument).

Is my understanding correct? At the same time let me make sure to add protobuf as the default value.

Also, if we want protocol to act as a default argument then it has to be placed at the end.

I think you can do what you did on the other and kept it at the end

docarray/base_doc/mixins/io.py

samsja

Thanks for you contribution ! Indeed litteral makes more sense in most of the case.

Two overall comments:

Lets keep default value
If possible we should not change the order of the parameters of existing function

samsja · 2023-07-04T07:21:34Z

docarray/store/helpers.py

@@ -167,13 +183,28 @@ def _to_binary_stream(


 def _from_binary_stream(
+    self, protocol: Literal['pickle', 'protobuf'],


same here we can do like you id in the other

Facing this error When I try to commit the changes. Any help on the same appreciated.

@samsja Can you have a look at this? Meanwhile let me work on the function implementation.

samsja

It starts to look very good.

One idea, maybe we could wrap the Literal['protobuf','pickle'] into a more formal type that we could call each time instead or repeating it ?

Something like this:

Protocols = Literal['pickle','protobuf']

def my_func(protocol: Protocol):
   ...

Wdyt ?

srini047 · 2023-07-04T11:05:00Z

It starts to look very good.

One idea, maybe we could wrap the Literal['protobuf','pickle'] into a more formal type that we could call each time instead or repeating it ?

Something like this:
Protocols = Literal['pickle','protobuf']

def my_func(protocol: Protocol):
   ...
Wdyt ?

It increases the modularity of the code and may be easier to maintain as well, what do you say?

samsja · 2023-07-04T11:40:28Z

It starts to look very good.
One idea, maybe we could wrap the Literal['protobuf','pickle'] into a more formal type that we could call each time instead or repeating it ?
Something like this:
Protocols = Literal['pickle','protobuf']

def my_func(protocol: Protocol):
   ...
Wdyt ?
It increases the modularity of the code and may be easier to maintain as well, what do you say?

then lets do it

srini047 · 2023-07-05T11:58:19Z

Hey @samsja!

Please have a look at the approach if this helps. Or else let me revert to any other logic
Also, I need help in docarray/store/helpers.py and I have tagged you in the issue I am facing.

docarray/base_doc/mixins/io.py

samsja · 2023-07-14T07:19:19Z

@srini047 can you install the pre commit hook and apply black, ruff and mypy for the CI to pass !

Signed-off-by: srini047 <srinikethcr7@gmail.com>

Signed-off-by: Joan Fontanals Martinez <joan.martinez@jina.ai> Signed-off-by: srini047 <srinikethcr7@gmail.com>

build(JoanFM): release 0.35.0 Signed-off-by: srini047 <srinikethcr7@gmail.com>

Signed-off-by: srini047 <srinikethcr7@gmail.com>

Signed-off-by: Sriniketh J <81156510+srini047@users.noreply.github.com>

codecov · 2023-07-21T17:23:04Z

Codecov Report

Patch coverage: 18.75% and project coverage change: -22.37 ⚠️

Comparison is base (304a4e9) 85.55% compared to head (b7664a4) 63.18%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1680       +/-   ##
===========================================
- Coverage   85.55%   63.18%   -22.37%     
===========================================
  Files         133      133               
  Lines        8585     8598       +13     
===========================================
- Hits         7345     5433     -1912     
- Misses       1240     3165     +1925

Flag	Coverage Δ
docarray	`63.18% <18.75%> (-22.37%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
docarray/store/helpers.py	`22.50% <14.28%> (-64.54%)`	⬇️
docarray/base_doc/mixins/io.py	`78.49% <50.00%> (-11.24%)`	⬇️

... and 72 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

samsja reviewed Jul 3, 2023

View reviewed changes

docarray/base_doc/mixins/io.py Outdated Show resolved Hide resolved

samsja requested changes Jul 3, 2023

View reviewed changes

samsja reviewed Jul 4, 2023

View reviewed changes

samsja reviewed Jul 5, 2023

View reviewed changes

docarray/base_doc/mixins/io.py Show resolved Hide resolved

srini047 and others added 7 commits July 21, 2023 22:47

refactor: Literals in type hints

6ee8b3c

Signed-off-by: srini047 <srinikethcr7@gmail.com>

fix: default value error

b239c2a

Signed-off-by: srini047 <srinikethcr7@gmail.com>

ci: pin pydantic version (docarray#1682)

3faaec5

Signed-off-by: Joan Fontanals Martinez <joan.martinez@jina.ai> Signed-off-by: srini047 <srinikethcr7@gmail.com>

chore: update version to 0.35.0 (docarray#1684)

b93a676

Signed-off-by: Joan Fontanals Martinez <joan.martinez@jina.ai> Signed-off-by: srini047 <srinikethcr7@gmail.com>

chore(version): the next version will be 0.35.1

03a0125

build(JoanFM): release 0.35.0 Signed-off-by: srini047 <srinikethcr7@gmail.com>

implement: function based approach to literals

1a27979

Signed-off-by: srini047 <srinikethcr7@gmail.com>

fix: ruff & black ci err

661686f

Signed-off-by: srini047 <srinikethcr7@gmail.com>

srini047 force-pushed the refactor-literal-type-hints branch from 92d3b4f to 661686f Compare July 21, 2023 17:17

Merge branch 'main' into refactor-literal-type-hints

b7664a4

Signed-off-by: Sriniketh J <81156510+srini047@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: use `Literals` in type hints #1680

Refactor: use `Literals` in type hints #1680

srini047 commented Jun 28, 2023

JoanFM commented Jun 28, 2023

srini047 commented Jun 29, 2023 •

edited

JoanFM commented Jun 29, 2023

srini047 commented Jun 29, 2023

samsja Jul 3, 2023

srini047 Jul 4, 2023

srini047 Jul 4, 2023

samsja Jul 4, 2023

samsja left a comment

samsja Jul 4, 2023

srini047 Jul 4, 2023

srini047 Jul 4, 2023

samsja left a comment

srini047 commented Jul 4, 2023 •

edited

samsja commented Jul 4, 2023

srini047 commented Jul 5, 2023

samsja commented Jul 14, 2023

codecov bot commented Jul 21, 2023 •

edited

		@@ -131,12 +132,27 @@ def close(self):


		def _to_binary_stream(
		self, protocol: Literal['pickle', 'protobuf'],

		@@ -167,13 +183,28 @@ def _to_binary_stream(


		def _from_binary_stream(
		self, protocol: Literal['pickle', 'protobuf'],

Refactor: use Literals in type hints #1680

Are you sure you want to change the base?

Refactor: use Literals in type hints #1680

Conversation

srini047 commented Jun 28, 2023

JoanFM commented Jun 28, 2023

srini047 commented Jun 29, 2023 • edited

JoanFM commented Jun 29, 2023

srini047 commented Jun 29, 2023

samsja Jul 3, 2023

Choose a reason for hiding this comment

srini047 Jul 4, 2023

Choose a reason for hiding this comment

srini047 Jul 4, 2023

Choose a reason for hiding this comment

samsja Jul 4, 2023

Choose a reason for hiding this comment

samsja left a comment

Choose a reason for hiding this comment

samsja Jul 4, 2023

Choose a reason for hiding this comment

srini047 Jul 4, 2023

Choose a reason for hiding this comment

srini047 Jul 4, 2023

Choose a reason for hiding this comment

samsja left a comment

Choose a reason for hiding this comment

srini047 commented Jul 4, 2023 • edited

samsja commented Jul 4, 2023

srini047 commented Jul 5, 2023

samsja commented Jul 14, 2023

codecov bot commented Jul 21, 2023 • edited

Codecov Report

Refactor: use `Literals` in type hints #1680

Refactor: use `Literals` in type hints #1680

srini047 commented Jun 29, 2023 •

edited

srini047 commented Jul 4, 2023 •

edited

codecov bot commented Jul 21, 2023 •

edited