Description

JohannesMessner

opened on Jun 26, 2023

Issue body actions

Right now we don't really use Literal types anywhere, but we should.
For example, see the following signature:

    @classmethod
    def from_bytes(
        cls: Type[T],
        data: bytes,
        protocol: str = 'protobuf',
        compress: Optional[str] = None,
    ) -> T:
        """Build Document object from binary bytes

        :param data: binary bytes
        :param protocol: protocol to use. It can be 'pickle' or 'protobuf'
        :param compress: compress method to use
        :return: a Document object
        """

Here, protocol can only be 'pickle' or 'protobuf', so Literal['pickle', 'protobuf'] would be the most suitable type hint.
Changing that will require multiple other changes in order to keep mypy happy, since other code relies on protocol being str.

This issue is about adjusting the above, and identifying and fixing other similar instances (if they exists).

Activity

1. JohannesMessner

   added this to DocArray backlogon Jun 26, 2023
2. JohannesMessner

   converted this from a draft issue on Jun 26, 2023
3. JohannesMessner

   added

   good-first-issueSuitable as your first contribution to DocArray!Suitable as your first contribution to DocArray!

   area/typing

   on Jun 26, 2023
4. srini047 commented on Jun 26, 2023

   

   srini047

   on Jun 26, 2023

   Contributor

   More actions

   Hey, @JohannesMessner I would like to work on this issue.
5. JoanFM commented on Jun 26, 2023

   

   JoanFM

   on Jun 26, 2023

   Member

   More actions

   hey @srini047 ,

   It would be great if you can take up this issue.

   Reacted by Sriniketh J
6. JoanFM

   assigned

   srini047

   on Jun 26, 2023
7. JoanFM

   moved this from Todo/Bug to In Progress in DocArray backlogon Jul 5, 2023
8. JoanFM

   moved this from In Progress to In progress by community in DocArray backlogon Jul 5, 2023
9. bpshaver commented on Oct 23, 2023

   

   bpshaver

   on Oct 23, 2023

   Contributor

   More actions

   Is protobuf-array also a possible value for protocol in the intended context? (If not... maybe the parameter name protocol should be used to refer to two different things.)
10. bpshaver commented on Oct 23, 2023

    

    bpshaver

    on Oct 23, 2023

    Contributor

    More actions

    Opened a PR.

    I'm in favor of an enum over a Literal consisting of different strings, but I can see how the latter is easier for end users. I can seek out other instances of this same issue if this PR looks good.
11. JohannesMessner commented on Oct 24, 2023

    

    JohannesMessner

    on Oct 24, 2023

    MemberAuthor

    More actions

    I can seek out other instances of this same issue if this PR looks good.

    The PR looks good, helping us with further instances of this problem would be great!

    Reacted by Ben Shaver
12. JoanFM commented on Oct 24, 2023

    

    JoanFM

    on Oct 24, 2023

    Member

    More actions

    Yes, it would be nice to make sure there are other instances or if we can close this
13. JoanFM

    unassigned

    srini047

    on Oct 24, 2023
14. bpshaver commented on Oct 24, 2023

    

    bpshaver

    on Oct 24, 2023

    Contributor

    More actions

    I believe these parameters are candidates for similar treatment:

    • audio_codec

    • audio_format

    • video_codec

    But there may be more

    Reacted by Joan Fontanals
15. JoanFM commented on Oct 24, 2023

    

    JoanFM

    on Oct 24, 2023

    Member

    More actions

    They do seem nice candidates sure

Issue actions