Skip to content

Unify package and archive modes#273

Merged
sywhang merged 2 commits into
uber-go:mainfrom
podtserkovskiy:unify_package_and_archive_step2
Oct 4, 2025
Merged

Unify package and archive modes#273
sywhang merged 2 commits into
uber-go:mainfrom
podtserkovskiy:unify_package_and_archive_step2

Conversation

@podtserkovskiy

@podtserkovskiy podtserkovskiy commented Aug 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #272

@podtserkovskiy

podtserkovskiy commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

cc @JamyDev

@JamyDev

JamyDev commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

Seems sane to me. Tagging @sywhang for review.

@podtserkovskiy podtserkovskiy force-pushed the unify_package_and_archive_step2 branch from fd4fd0f to 030b99f Compare August 12, 2025 23:25
@podtserkovskiy

podtserkovskiy commented Aug 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@sywhang @linzhp Could you please review this PR?
It also unblocks Go1.25 compatibility golang/go#74462

I have two commits to simplify review

  • step1: re-uses "package" implementation for archive by removing receiver for methods
  • step2: makes both "package" and "archive" to work with ExportFile + upgrades x/tools for modern compiler support

pitabwire
pitabwire reviewed Aug 18, 2025
View reviewed changes
Comment thread go.mod Outdated
@podtserkovskiy podtserkovskiy force-pushed the unify_package_and_archive_step2 branch from 030b99f to f142e57 Compare August 19, 2025 11:11
@podtserkovskiy

podtserkovskiy commented Aug 22, 2025

Copy link
Copy Markdown
Contributor Author

Hey folks, is there a chance to get this PR merged?

sywhang
sywhang approved these changes Oct 3, 2025
View reviewed changes

@sywhang sywhang left a comment •
edited
Loading

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.

Thanks for this PR, and sorry for the delay in response. The Uber Go team is fairly occupied lately so we've got a bit of catch up reviews to do.

Just took a look at this PR and it looks good, but there are some behavioral changes in archive mode that's being "fixed" as a result of the code merging that might be worth adding to CHANGELOG:

  • we now error out on type constraints in archive mode.
  • we now handle type aliases explicitly in archive mode.

Feel free to modify CHANGELOG, otherwise I plan on merging this tomorrow morning and will be adding these ourselves later before releasing.

@podtserkovskiy podtserkovskiy force-pushed the unify_package_and_archive_step2 branch from f142e57 to 9098d3b Compare October 3, 2025 10:39
@podtserkovskiy

podtserkovskiy commented Oct 3, 2025

Copy link
Copy Markdown
Contributor Author

No worries, thank you for the review. I've added information about this change to the CHANGELOG.

sywhang
sywhang approved these changes Oct 4, 2025
View reviewed changes

@sywhang sywhang 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.

thanks!

@sywhang sywhang merged commit ca5300b into uber-go:main Oct 4, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sywhang sywhang sywhang approved these changes

+1 more reviewer

@pitabwire pitabwire pitabwire left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unify package and archive modes

4 participants

@podtserkovskiy @JamyDev @pitabwire @sywhang