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

refactor(archive): replace FileTypes enum with const object #3845

Closed
wants to merge 1 commit into from

Conversation

realpha
Copy link
Contributor

@realpha realpha commented Nov 23, 2023

Refers to this issue

This change was a little bit tricky because the exposed interfaces do not enforce FileTypes in the input they receive. The old enum also wasn't exposed via the module API.
Consequently, the implementations also don't really trust their input but it seems that failing to provide a correct filetype doesn't necessarily lead to a panic.

I've tried to make this change as minimally invasive as possible, but IMO it doesn't improve anything. Maybe this refactor should be completed in tandem with the stabilization of the archive module API to actually contribute something to the overall maintainability and quality of this module.

@realpha
Copy link
Contributor Author

realpha commented Nov 23, 2023

@iuioiua

@iuioiua
Copy link
Contributor

iuioiua commented Nov 23, 2023

This change was a little bit tricky because the exposed interfaces do not enforce FileTypes in the input they receive.

Are you talking about TarInfo.type being of type string instead of keyof typeof FileTypes? If so, this implementation will be deprecated once #1985 lands, and the new implementation addresses that.

Consequently, the implementations also don't really trust their input but it seems that failing to provide a correct filetype doesn't necessarily lead to a panic.

I've tried to make this change as minimally invasive as possible, but IMO it doesn't improve anything. Maybe this refactor should be completed in tandem with the stabilization of the archive module API to actually contribute something to the overall maintainability and quality of this module.

Let's skip this enum, as the current implementation will be deprecated once the new implementation lands. I'll close this PR and remove it from the list. Are you able to create a new issue regarding panicking when providing the incorrect file type? Either way, thank you for your help!

@iuioiua iuioiua closed this Nov 23, 2023
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.

2 participants