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

[std/archive] Improve docs and general code cleanup #3593

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

cknight
Copy link
Contributor

@cknight cknight commented Aug 31, 2023

std/archive needs some TLC. This is the first in a series of improvements to this module. Specifically, this PR adds or improves documentation and makes cosmetic code changes primarily for readability.

@cknight cknight requested a review from kt3k as a code owner August 31, 2023 07:59
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kt3k
Copy link
Member

kt3k commented Aug 31, 2023

BTW we've been trying to redesign this module. This module is based on legacy Reader/Writer interfaces, but we've been deprecating them in favor of ReadableStream/WritableStream interfaces.

We tried to make std/archive/tar to use ReadableStream/WritableStream in #1985, but it needed to depend on unfinished web stream proposal, and we couldn't land it.

We'd appreciate if you could make progress along with this direction.

@kt3k kt3k merged commit 7fc5cda into denoland:main Aug 31, 2023
@cknight
Copy link
Contributor Author

cknight commented Aug 31, 2023

Thanks @kt3k, that's good to hear. My further proposals for this module are around quality of life usage (adding compression and convenience functions to make the API much more user friendly). They build upon the existing Reader usage however. Given this, and the desire to redesign the module, I can hold off if the redesign is now able to be progressed. I'd be happy to help progress, though streams are new to me. However, it looks like the unfinished web stream proposal is still unfinished and therefore continues to block the completion of #1985?

@kt3k
Copy link
Member

kt3k commented Sep 1, 2023

Yeah, it still blocks #1985, but I was wondering if there's some alternative way which avoids depending on the proposal, but still replaces Reader/Writer with web streams..

@iuioiua
Copy link
Contributor

iuioiua commented Sep 1, 2023

Yeah, it still blocks #1985, but I was wondering if there's some alternative way which avoids depending on the proposal, but still replaces Reader/Writer with web streams..

Apparently not

@cknight
Copy link
Contributor Author

cknight commented Sep 1, 2023

@crowlKats Are you able to articulate in layman's terms what is incomplete with your efforts in #1985? I've read your PR and the corresponding incomplete spec proposal but am missing the gap it plugs.

@crowlKats
Copy link
Member

well, the spec proposal itself needs to land to be able to have #1985

@cknight
Copy link
Contributor Author

cknight commented Sep 1, 2023

Ah, are you saying then that the code you've written matches the proposed spec, but we can't land the PR until the spec lands first? Makes sense, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants