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

Change Dict field names to _keys and _vals #49095

Open
CameronBieganek opened this issue Mar 22, 2023 · 8 comments
Open

Change Dict field names to _keys and _vals #49095

CameronBieganek opened this issue Mar 22, 2023 · 8 comments
Labels
breaking This change will break code

Comments

@CameronBieganek
Copy link
Contributor

To avoid new users accidentally accessing the internals of a dictionary via d.keys or d.vals (e.g., if they're coming from an OOP language), it's probably a good idea to change the names of those fields to _keys and _vals.

Inspired by this question on Discourse:
https://discourse.julialang.org/t/dictionary-showing-invalid-keys/96422

@oscardssmith oscardssmith added the good first issue Indicates a good issue for first-time contributors to Julia label Mar 22, 2023
@KristofferC KristofferC removed the good first issue Indicates a good issue for first-time contributors to Julia label Mar 22, 2023
@KristofferC
Copy link
Member

This will break a lot of code out there. Does not seem worth the churn.

@oscardssmith
Copy link
Member

do you have an example of someone using this outside of base?

@jishnub
Copy link
Contributor

jishnub commented Mar 22, 2023

This sounds like something a linter might warn against

@petvana
Copy link
Member

petvana commented Mar 22, 2023

The number of packages that uses internal data is quite large. Some of them extends the Dict, so they have a good reason to use it. You can find some of them by checking in #44332, where I have tried to make the code backward compatible.

@DNF2
Copy link

DNF2 commented Mar 22, 2023

do you have an example of someone using this outside of base?

https://discourse.julialang.org/t/dictionary-showing-invalid-keys/96422/1

Does this mean, however, that all non-public fields should now have a leading underscore? Otherwise, this becomes inconsistent and confusing.

@CameronBieganek
Copy link
Contributor Author

Does this mean, however, that all non-public fields should now have a leading underscore? Otherwise, this becomes inconsistent and confusing.

No, there is no such proscription. As always, fields can be named anything we want. I don't think renaming two fields is a cause for confusion.

This will break a lot of code out there. Does not seem worth the churn.

It's unfortunate that we can't make non-breaking changes in Base without breaking things.

@KristofferC
Copy link
Member

It's unfortunate that we can't make non-breaking changes in Base without breaking things.

Yes, a pragmatic view always has to be taken. But you could try open a PR, run PkgEval, see the level of breakage and maybe fix those packages etc.

@udohjeremiah udohjeremiah added the breaking This change will break code label Apr 19, 2023
@DNF2
Copy link

DNF2 commented Apr 20, 2023

No, there is no such proscription. As always, fields can be named anything we want. I don't think renaming two fields is a cause for confusion.

What I meant is this: If one starts prepending underscores to signal that some fields are internal, what does that say about the fields that do not have leading underscores?

This should either be a convention to be followed more generally, or it risks encouraging people to access internal fields in some cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

No branches or pull requests

7 participants