GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat: use literal in type hints by bpshaver · Pull Request #1827 · docarray/docarray

This repository was archived by the owner on Sep 9, 2026. It is now read-only.

Conversation

@bpshaver

@bpshaver bpshaver commented Oct 23, 2023

Copy link
Copy Markdown
Contributor

Closes the immediate type issue in #1670

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bpshaver
bpshaver commented Oct 23, 2023
View reviewed changes
Comment thread docarray/utils/_internal/misc.py
'pymilvus': '"docarray[milvus]"',
}

ProtocolType = Literal[

@bpshaver bpshaver Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JoanFM JoanFM Oct 23, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bpshaver bpshaver mentioned this pull request Oct 23, 2023
Refactor: use Literal in type hints #1670
Open
JoanFM
JoanFM approved these changes Oct 23, 2023
View reviewed changes
Comment thread docarray/utils/_internal/misc.py
'pymilvus': '"docarray[milvus]"',
}

ProtocolType = Literal[

@JoanFM JoanFM Oct 23, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JoanFM

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bpshaver

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bpshaver added 4 commits October 23, 2023 16:27
@bpshaver
feat(types): added ProtocolType to give more constrained type to `pro…
784fc40 
…tocol` parameters

Signed-off-by: Ben Shaver <[email protected]>
@bpshaver
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 <[email protected]>
@bpshaver
refactor(types): changed instances of protocol parameter to use new P…
1977708 
…rotocolType

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

bpshaver commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

@JoanFM Commits are now signed!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bpshaver added 2 commits October 23, 2023 16:47
@bpshaver
fix(types): moving import ProtocolType from under TYPE_CHECKING box
d60ebe6 
Signed-off-by: Ben Shaver <[email protected]>
@bpshaver
fix(types): moving import of ProtocolType from under TYPE_CHECKING block
bbb8939 
Signed-off-by: Ben Shaver <[email protected]>
@JoanFM JoanFM changed the title Feat/#1670/use literal in type hints feat: use literal in type hints Oct 23, 2023
@JoanFM
JoanFM requested a review from JohannesMessner October 23, 2023 21:04
@codecov

codecov Bot commented Oct 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JohannesMessner
JohannesMessner approved these changes Oct 24, 2023
View reviewed changes
@JohannesMessner

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JoanFM
JoanFM merged commit 522811f into docarray:main Oct 24, 2023
@bpshaver
bpshaver deleted the feat/#1670/use-literal-in-type-hints branch October 24, 2023 14:27
@JoanFM JoanFM mentioned this pull request Dec 22, 2023
Release Note #1842
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@JoanFM JoanFM JoanFM approved these changes
@JohannesMessner JohannesMessner JohannesMessner approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@bpshaver @JoanFM @JohannesMessner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.