Skip to content

refactor: move types to zip format folder#827

Open
Its-Just-Nans wants to merge 8 commits intomasterfrom
refactor-zip-format
Open

refactor: move types to zip format folder#827
Its-Just-Nans wants to merge 8 commits intomasterfrom
refactor-zip-format

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

No description provided.

@Its-Just-Nans Its-Just-Nans self-assigned this May 7, 2026
Copy link
Copy Markdown
Contributor

@amazon-q-developer amazon-q-developer Bot left a comment

Choose a reason for hiding this comment

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

This refactoring successfully reorganizes AES/encryption-related types and the System enum into dedicated modules under src/format/. The changes are clean with no logic modifications - purely structural improvements for better code organization. All imports have been updated correctly throughout the codebase. No issues found that would block merge.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the ZIP format specifications by moving AES and flag-related enums into a new format module and updating serialization logic for extra fields to use the Write trait directly. Key changes include the removal of Pod implementations for encryption structures and the addition of a new test case for AES version thresholds. Review feedback points out a missing import for size_of that would cause a compilation error and recommends using constants in tests to improve maintainability.

Comment thread src/extra_fields/aex_encryption.rs
Comment thread src/extra_fields/aex_encryption.rs
@Pr0methean Pr0methean force-pushed the refactor-zip-format branch from 3391ebe to 937d541 Compare May 9, 2026 00:17
Comment thread src/read/stream.rs Fixed
Copy link
Copy Markdown
Member

@Pr0methean Pr0methean left a comment

Choose a reason for hiding this comment

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

Generally looks good, but we may as well do some refactors for the code that's moving into format/ since we need to cut a major release anyway.

Comment thread src/format/aes.rs
Comment on lines +19 to +21
pub const fn as_u16(self) -> u16 {
self as u16
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method seems unnecessary. Can't it be either inlined or replaced with the From<AesVendorVersion> for u16 impl below?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am now using as_u16() inside the From

Copy link
Copy Markdown
Member

@Pr0methean Pr0methean May 9, 2026

Choose a reason for hiding this comment

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

Why not just use as u16 inside the From and wherever else it's currently used, so AesVendorVersion can have one less method?

Comment thread src/format/aes.rs
Comment thread src/format/flags.rs Outdated
@Pr0methean Pr0methean mentioned this pull request May 9, 2026
2 tasks
Comment thread src/format/aes.rs
Comment on lines +19 to +21
pub const fn as_u16(self) -> u16 {
self as u16
}
Copy link
Copy Markdown
Member

@Pr0methean Pr0methean May 9, 2026

Choose a reason for hiding this comment

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

Why not just use as u16 inside the From and wherever else it's currently used, so AesVendorVersion can have one less method?

@Its-Just-Nans
Copy link
Copy Markdown
Member Author

Its-Just-Nans commented May 9, 2026 via email

@Pr0methean
Copy link
Copy Markdown
Member

I find it easier to have one method for all conversion to u16 In case we change how we do the conversion or for readability

In what circumstances would it ever possibly change? And in what sense is x.as_u16() less readable than x as u16? (Functional pipelines can already just use u16::from instead of a closure.)

@Its-Just-Nans
Copy link
Copy Markdown
Member Author

I find it easier to have one method for all conversion to u16 In case we change how we do the conversion or for readability

In what circumstances would it ever possibly change? And in what sense is x.as_u16() less readable than x as u16? (Functional pipelines can already just use u16::from instead of a closure.)

I think there are no real argument, but some of mine

  • I find that as_u16() is more readable
  • I find that as_u16() is more explicit since it means we don't cast or anything
  • If there are more type of VendorVersion that need more complex u16 representation, a specific function as_u16() could be easily changed

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