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 foo[:bar] interfaces to foo.bar #25159

Closed
stevengj opened this issue Dec 18, 2017 · 5 comments
Closed

change foo[:bar] interfaces to foo.bar #25159

stevengj opened this issue Dec 18, 2017 · 5 comments
Labels
linear algebra Linear algebra needs decision A decision on this change is needed

Comments

@stevengj
Copy link
Member

Now that #24960 is merged, it's important that we think through its implications. In particular, for types where we used a foo[:bar] API, we should strongly consider changing it to foo.bar using getproperty.

In particular, I'm thinking of factorization objects like qrfact, where we currently use QR[:Q]. It seems like QR.Q would be a lot more natural.

@stevengj stevengj added needs decision A decision on this change is needed linear algebra Linear algebra labels Dec 18, 2017
@andreasnoack
Copy link
Member

I'm almost done with this so I'll open a PR shortly.

@stevengj
Copy link
Member Author

stevengj commented Dec 18, 2017

Besides factorization objects, the only other place where this idiom seems to be used is getindex(m::RegexMatch, name::Symbol), which should possibly be changed too?

@StefanKarpinski
Copy link
Member

I can work on that one, since I've been doing some other work that touches regexes anyway.

@StefanKarpinski
Copy link
Member

Although that's a little different since we do want to do m.match and m.captures to get at what the match object matched and captured. I guess I could change m.captures to a RegexCaptures object that is both indexable like an array of strings and that you can do m.captures.name on. But I'm not sure that's worth it since writing m[:name] is quite a bit shorter anyway.

@vtjnash
Copy link
Member

vtjnash commented Dec 18, 2017

I think we should keep m[:name] for Regex (or use m["name"]?). The QR change seems pretty clearly better though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

4 participants