Skip to content

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

Merged
kt3k merged 1 commit into
denoland:mainfrom
cknight:archiveDocs
Aug 31, 2023
Merged

[std/archive] Improve docs and general code cleanup#3593
kt3k merged 1 commit into
denoland:mainfrom
cknight:archiveDocs

Conversation

@cknight

@cknight cknight commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

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

@kt3k kt3k left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kt3k

kt3k commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

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

cknight commented Aug 31, 2023

Copy link
Copy Markdown
Contributor Author

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

kt3k commented Sep 1, 2023

Copy link
Copy Markdown
Contributor

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

iuioiua commented Sep 1, 2023

Copy link
Copy Markdown
Contributor

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

cknight commented Sep 1, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Member

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

@cknight

cknight commented Sep 1, 2023

Copy link
Copy Markdown
Contributor Author

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.

4 participants