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

RFC: remove deprecation of getproperty on Pairs #41027

Closed
wants to merge 1 commit into from

Conversation

KristofferC
Copy link
Sponsor Member

This causes hundred+ of packages (looking at #39448 (comment)) to start giving deprecation warnings by default. The motivation for the feature (#25750) also seems highly speculative so making something disruptive based on that doesn't feel like it is a good tradeoff.

@KristofferC KristofferC force-pushed the kc/rev_deprecation_pair_property branch from c975f2d to 3c5411b Compare May 31, 2021 15:25
@KristofferC KristofferC changed the title RFC: remove deprecatoin of getproperty on Pair RFC: remove deprecation of getproperty on Pair May 31, 2021
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

It is not necessary to delete the deprecation. It is why we have deprecations, particularly for packages that are buggy.

@KristofferC
Copy link
Sponsor Member Author

It is not necessary to delete the deprecation. It is why we have deprecations, particularly for packages that are buggy.

Please address the comment in the OP; it is never claimed that it is a necessity. It is claiming that the annoyance it brings for users is not worth the benefit it provides. That trade-off has to always be considered when implementing something, no matter if it is internal or not.

@simeonschaub simeonschaub changed the title RFC: remove deprecation of getproperty on Pair RFC: remove deprecation of getproperty on Pairs Jun 1, 2021
@simeonschaub
Copy link
Member

Would just removing the force=true be a reasonable compromise? That way, it would only show up in tests, not regular usage. We don't document the internal fields of Pairs anywhere, so I don't think a deprecation here is unreasonable.

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Jun 18, 2021

The whole point is that

We don't document the internal fields of Pairs anywhere, so I don't think a deprecation here is unreasonable

means there is no need to add a deprecation warning. It is completely arbitrary why this specific field has a deprecation warning and now e.g. PkgEval logs are spammed with this, it will show up in old package versions forever, and making it silent means there is still a performance problem with it and this is not unlikely to be used in hot code. All around, nothing has really been gained and a bunch of pain in the butt has been created.

@simeonschaub
Copy link
Member

Can PkgEval just disable deprecation warnings in tests?

means there is no need to add a deprecation warning.

So you think this should just be an error? The problem is that this would be quite disruptive, since a lot of packages rely on this internal detail.

@KristofferC
Copy link
Sponsor Member Author

So you think this should just be an error?

No, I think nothing should be done. Just like in #41018.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 29, 2021

How does this compare to #41018? We can restore those named bits if you want, I was probably going to bring them back eventually via compiler features anyways. That contrasts to this PR, where we plan to make this go away permanently in the next release.

@KristofferC
Copy link
Sponsor Member Author

where we plan to make this go away permanently in the next release.

Are we?

#25750 (comment)

#25750 (comment)

#25750 (comment)

etc.

@giordano giordano deleted the kc/rev_deprecation_pair_property branch June 7, 2022 19:22
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