FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: use literal in type hints by bpshaver · Pull Request #1827 · docarray/docarray · GitHub

/ docarray Public

feat: use literal in type hints - #1827

Merged
JoanFM merged 6 commits into
docarray:mainfrom
bpshaver:feat/#1670/use-literal-in-type-hints
Oct 24, 2023
ConversationCommits (6)ChecksFiles changed
Merged

feat: use literal in type hints#1827
JoanFM merged 6 commits into
docarray:mainfrom
bpshaver:feat/#1670/use-literal-in-type-hints

Conversation

bpshaver commented Oct 23, 2023

Copy link
Copy Markdown
Contributor

Closes the immediate type issue in #1670

bpshaver commented Oct 23, 2023
View reviewed changes
Comment thread docarray/utils/_internal/misc.py
'pymilvus': '"docarray[milvus]"',
}

ProtocolType = Literal[

bpshaver Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Followed the ALLOWED_PROTOCOLS constant at https://github.com/docarray/docarray/blob/main/docarray/array/doc_list/io.py#L54C1-L54C1

It makes me uncomfortable to define the same information twice, so I can investigate combining the ProtocolType and ALLOWED_PROTOCOLS objects, even though one is a type object and the other is a set of strings.

JoanFM Oct 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In a follow-up PR you can define ALLOWED_PROTOCOLS in relation to these types

bpshaver mentioned this pull request Oct 23, 2023
Open
JoanFM approved these changes Oct 23, 2023
View reviewed changes
Comment thread docarray/utils/_internal/misc.py
'pymilvus': '"docarray[milvus]"',
}

ProtocolType = Literal[

JoanFM Oct 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In a follow-up PR you can define ALLOWED_PROTOCOLS in relation to these types

JoanFM commented Oct 23, 2023

Copy link
Copy Markdown
Member

Hello @bpshaver,

We need for you to sign off your commits so that we can introduce your contributions. Thanks for the great one btw

bpshaver commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

Hello @bpshaver,

We need for you to sign off your commits so that we can introduce your contributions. Thanks for the great one btw

Drats! I signed off on the last commit but not the first two. Let me see about amending the older ones.

Random question: is there a config file mypy should be detecting? I run mypy on the command line and as an LSP, and I'm seeing a lot of errors related to third party libraries that I'm used to ignoring in a config file.

feat(types): added ProtocolType to give more constrained type to `pro…
784fc40 
…tocol` parameters

Signed-off-by: Ben Shaver <benpshaver@gmail.com>
feat(types): expanded set of allowed protocol types
9dc0eff 
Followed the ALLOWED_PROTOCOLS constant at https://github.com/docarray/docarray/blob/main/docarray/array/doc_list/io.py#L54C1-L54C1

Signed-off-by: Ben Shaver <benpshaver@gmail.com>
refactor(types): changed instances of protocol parameter to use new P…
1977708 
…rotocolType

Signed-off-by: Ben Shaver <benpshaver@gmail.com>
chore(fmt): auto-formatting
61eb05f 
Signed-off-by: Ben Shaver <benpshaver@gmail.com>
bpshaver force-pushed the feat/#1670/use-literal-in-type-hints branch from 7634fe0 to 61eb05f Compare October 23, 2023 20:27

bpshaver commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

@JoanFM Commits are now signed!

JoanFM changed the title Feat/#1670/use literal in type hints feat: use literal in type hints Oct 23, 2023

codecov Bot commented Oct 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d5d928b) 85.04% compared to head (bbb8939) 85.05%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1827   +/-   ##
=======================================
  Coverage   85.04%   85.05%           
=======================================
  Files         135      135           
  Lines        9028     9031    +3     
=======================================
+ Hits         7678     7681    +3     
  Misses       1350     1350
Flag Coverage Δ
docarray 85.05% <81.81%> (+<0.01%) ⬆️

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

Files Coverage Δ
docarray/array/doc_vec/io.py 94.21% <100.00%> (+0.03%) ⬆️
docarray/base_doc/mixins/io.py 90.73% <100.00%> (ø)
docarray/store/helpers.py 60.90% <100.00%> (+0.35%) ⬆️
docarray/utils/_internal/misc.py 66.12% <100.00%> (+0.55%) ⬆️
docarray/array/doc_list/io.py 81.55% <60.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JohannesMessner commented Oct 24, 2023

Copy link
Copy Markdown
Member

Random question: is there a config file mypy should be detecting? I run mypy on the command line and as an LSP, and I'm seeing a lot of errors related to third party libraries that I'm used to ignoring in a config file.

My understanding is that this should be defined in pyprojects.toml.

JoanFM merged commit 522811f into docarray:main Oct 24, 2023
bpshaver deleted the feat/#1670/use-literal-in-type-hints branch October 24, 2023 14:27
JoanFM mentioned this pull request Dec 22, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JoanFM JoanFM approved these changes
JohannesMessner JohannesMessner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL