Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stub files for fastwarc #44

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

habibutsu
Copy link
Contributor

@habibutsu habibutsu commented Jul 14, 2024

Implementation #30: Of course, it would be better to generate it from Cython definition, but since it is not fully supported yet, and considering that the libraries are not updated often, this can be useful.

@phoerious
Copy link
Member

Is this really needed? Auto-complete should work with the string signature from the doc strings as well.

@habibutsu
Copy link
Contributor Author

It would be useful for VS Code users, as auto-completion is not working in the Pylance server without this.

@phoerious
Copy link
Member

I'm a bit worried about the double work of maintaining two different API descriptions, neither of which can be generated automatically.

@habibutsu
Copy link
Contributor Author

habibutsu commented Jul 15, 2024

Yes, I understand your concerns and agree that this is not the best solution. However, do you have any plans to implement new features or changes? I see that the library is quite stable, and it shouldn't be a problem to maintain this.

@phoerious
Copy link
Member

The FastWARC API itself is pretty stable, but the rest of Resiliparse would indeed be a hassle to maintain double.

There are plans to migrate a bunch of the core components to Rust. Some work has been done on this already, but only in the backend. I hope the Rust Python bindings make the generation of Stub files easier.

@habibutsu
Copy link
Contributor Author

You can release your future improved implementation with the new major version v0.15.x and keep the current implementation with v0.14.x.

@phoerious
Copy link
Member

I think it would be best if you could then also remove the string signatures from the affected attributes and functions. Back when I wrote it I had the choice between the two and decided for string signatures, because they are directly next to the actual signature. I think it's fine to switch to stub files now, but we shouldn't keep both.

@jonded94
Copy link
Contributor

jonded94 commented Aug 6, 2024

Hey, I'd also be very interested in having this library properly typehinted. This is not only because of language server reasons, but also for enabling using this library in any meaningful sense in an environment with mypy.

What is needed to merge this PR? With "remove the string signatures" you (@phoerious) mean for example these here: https://github.com/chatnoir-eu/chatnoir-resiliparse/blob/develop/fastwarc/fastwarc/tools.pyx#L58 ?

If that's the only thing blocking merging this PR, could we do it ASAP? Happy to help here, if needed.

@phoerious
Copy link
Member

It's these things: https://github.com/chatnoir-eu/chatnoir-resiliparse/blob/develop/fastwarc/fastwarc/stream_io.pyx#L362

There's nothing else blocking the merge.

@jonded94
Copy link
Contributor

jonded94 commented Aug 6, 2024

Just to be sure, you want to remove the type information only, i.e. this

    :param raw_stream: raw data stream
    :param compression_level: GZip compression level (for compression only)
    :type compression_level: int
    :param zlib: use raw deflate / zlib format instead of gzip
    :type zlib: bool

or, alternatively, move all of the docstrings into the stub files?

@phoerious
Copy link
Member

No, just the text signature lines like the one I linked.

cdef class GZipStream(CompressingStream):
    """
    __init__(self, raw_stream, compression_level=9, zlib=False)         <------- This one

    GZip :class:`IOStream` implementation.

    :param raw_stream: raw data stream
    :param compression_level: GZip compression level (for compression only)
    :type compression_level: int
    :param zlib: use raw deflate / zlib format instead of gzip
    :type zlib: bool
    """

@phoerious phoerious merged commit c361801 into chatnoir-eu:develop Aug 8, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants