Skip to content

Conversion to ESM #4

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

Merged
merged 9 commits into from
Jun 12, 2025
Merged

Conversion to ESM #4

merged 9 commits into from
Jun 12, 2025

Conversation

CDeltakai
Copy link
Contributor

@CDeltakai CDeltakai commented Jun 5, 2025

Description

This PR converts the repository to ESM.

Issues Fixed

Tasks

  • 1. Convert repository to ESM

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CDeltakai CDeltakai self-assigned this Jun 6, 2025
@aryanjassal aryanjassal requested a review from tegefaulkes June 11, 2025 07:20
@aryanjassal
Copy link
Member

I've taken over this PR. It should be complete now, but I've requested @tegefaulkes to review it once before merging. After merging, @CDeltakai can start integration into polykey for addressing MatrixAI/Polykey#822.

Comment on lines +18 to +26
name: utils.decodeFilePath(data),
type: utils.extractString(data, 156, 1),
mode: utils.extractOctal(data, 100, 8),
uid: utils.extractOctal(data, 108, 8),
gid: utils.extractOctal(data, 116, 8),
size: utils.extractOctal(data, 124, 12),
mtime: utils.extractOctal(data, 136, 12),
format: utils.extractString(data, 257, 6),
version: utils.extractString(data, 263, 2),

Choose a reason for hiding this comment

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

Out of scope for this PR, but there's a lot of magic numbers here.

Copy link
Member

Choose a reason for hiding this comment

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

I do have constants corresponding to these magic numbers. Might've slipped my mind during initial implementation.

Copy link

@tegefaulkes tegefaulkes left a comment

Choose a reason for hiding this comment

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

Looking fine.

@CDeltakai CDeltakai merged commit 07b8ef2 into master Jun 12, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Migrate js-virtualtar to ESM
3 participants