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

Remove some fields from Go context #4034

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

dzbarsky
Copy link
Contributor

What type of PR is this?
Starlark cleanup

What does this PR do? Why is it needed?
All of these are either unused or can easily be retrieved from another context field.
We were already retrieving them from both so this change makes things more consistent.

Which issues(s) does this PR fix?

Fixes #

Other notes for review

@sluongng
Copy link
Contributor

Im a bit iffy about this.

On one hand, this is a nice cleanup, probably overdue.

On the other hand, we have been advertising Go context wrapper as the canonical way to extend rules_go. I can see some downstream rules depending on go_ctx.go to access the binary directly for example.

Perhaps leaving the removed fields in the source as comments, each pointing to their replacement would help folks migrate more easily? WDYT?

@dzbarsky
Copy link
Contributor Author

Im a bit iffy about this.

On one hand, this is a nice cleanup, probably overdue.

On the other hand, we have been advertising Go context wrapper as the canonical way to extend rules_go. I can see some downstream rules depending on go_ctx.go to access the binary directly for example.

Perhaps leaving the removed fields in the source as comments, each pointing to their replacement would help folks migrate more easily? WDYT?

That's a good idea. If we want to avoid breakage, we can also add a deprecated_props kwarg or otherwise shim things so that internal rules get a slimmed down context and external world has a chance to migrate

@fmeum
Copy link
Member

fmeum commented Aug 15, 2024

I'm also in favor of cleaning up rules_go's own usages while still providing the same props, but moved to a separate "deprecated" section in the docs. We can call out their imminent removal in the release notes.

@dzbarsky dzbarsky force-pushed the zbarsky/context-cleanup branch 2 times, most recently from 791cf59 to 5a04f36 Compare August 15, 2024 15:55
@dzbarsky
Copy link
Contributor Author

I'm also in favor of cleaning up rules_go's own usages while still providing the same props, but moved to a separate "deprecated" section in the docs. We can call out their imminent removal in the release notes.

I've updated to allow opting out of deprecated properties and added to the docs. WDYT of this approach?

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

This is great, thanks for simplifying the migration!

@fmeum fmeum merged commit aa96a11 into bazel-contrib:master Aug 15, 2024
2 checks passed
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