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

.exists and .delete are internal methods and should be renamed #56

Closed
lizmat opened this issue Jun 21, 2013 · 8 comments
Closed

.exists and .delete are internal methods and should be renamed #56

lizmat opened this issue Jun 21, 2013 · 8 comments

Comments

@lizmat
Copy link
Contributor

lizmat commented Jun 21, 2013

I propose to rename .exists to .exists_at_key, and .delete to .delete_at_key, because they are really internal methods, not to be used in non-system oriented code. I propose I will do the work for this. This will be done in the following steps:

  1. create .exists_at_key in every class where need, as a dup of .exists
  2. modify all core code to use .exists_at_key
  3. modify tests using .exists to use :exists, or to use .exists_at_key if :exists is not possible
  4. add warning to .exists that it should be replaced with :exists.

*wait one month

  1. remove .exists from every class it exists.

Do the same in parallel for .delete.

Part of a recent dialogue.

http://irclog.perlgeek.de/perl6/2013-06-20#i_7226826
18:21 TimToady I don't mind something like .exists and .delete as internal methods, but they're more in the category of at_key and at_pos, not something that should be used much in Userland
18:22 so maybe a rename to exists_at_key and such
18:22 masak TimToady: this will break a lot of code.
18:22 masak a lot of code.
18:22 TimToady but all of those internals are subject to renegotiation as soon as S09 starts getting implemented
18:22 :exists is supposed to be S09-proof
18:23 we can't guarantee that for .exists
18:23 not without an explicit mapping of multidimensional args to single dimensional...
18:24 TimToady we know we have subscripts, and we know that we want to delete and test existence using subscript notation; the rest is still unspecced
18:26 TimToady what does this turn into: my Things @things[3,3,]; @things[1;2;$min..$max]:delete
18:27 TimToady er, my Things @things[3;3;
]; @things[1;2;$min..$max]:delete
18:27 TimToady s'posed to use ; consistently...
18:28 TimToady currently .exists only knows about single keys, afaik
18:29 TimToady we'd need a .exists that knows about LoLs, where each sublist can be lazy
18:29 the :exists form is more straightforward as soon as you get out of single dimensions
18:53 lizmat TimToady: I like the .exists_at_key idea, since .exists currently is really an at_key cut short
18:54 TimToady but again, what's a "key" look like when you add slicing and multidimensionality?
18:55 * TimToady isn't sure
18:55 lizmat If necessary, I can do this rename of .exists internally + test-suite
18:56 I agree with your argument about S09, :exists is the way to do this, as it will easily allow you to specify on which slice you want things to be applied
18:57 .exists should only handle a single key (as in perl 5), and to make it an "internal" method, rename it to ".exists_at_key"

@lizmat
Copy link
Contributor Author

lizmat commented Jun 21, 2013

[13:32:39] #56 .exists and .delete are internal methods and should be renamed
[13:33:20] :(
[13:33:46] jnthn: is that about .exists ?
[13:34:08] yeah
[13:34:47] I know it won't scale so well to multi-dim stuff, but "make the simple things simple and the hard things possible" and all that...
[13:35:34] are you worried about performance effects of using :exists rather than .exists ?
[13:35:43] No
[13:35:53] or are you worried about having to type .exists_at_key
[13:36:01] I just find .exists(...) more natural somehow
[13:36:39] I don't think renaming it will do much besides cause disruption.
[13:37:08] Same with delete
[13:37:21] r: my %h=a=>1,b=>2; say %h.exists()
[13:37:24] <+camelia> rakudo 49f111: OUTPUT«False␤»
[13:37:41] I don't think renaming it will do much besides cause disruption.
[13:37:43] +1
[13:37:52] I can see this being a "heart in the right place" action.
[13:37:59] but it will cause ecosystem havoc.
[13:38:09] aka, it is too late to change this?
[13:38:20] at the very least there should be a veeeery long deprecation cycle.
[13:38:51] in P6 currently, that would be 6 months ?
[13:38:53] I'm hard-pressed to think of anything non-trivial I've written that didn't use .exists or .delete
[13:38:55] Well, in some senses it's not too late to change anything until 6.0.0 is declared, but some things cost more early adopter goodwill than others to change.
[13:39:17] it would all have to be revisited and rewritten.
[13:39:41] "it" being core, or ecosystem ?
[13:40:07] ecosystem, I presume
[13:40:35] entire ecosystem rewritten! /o
[13:40:38] core is no problem, nor is roast
[13:41:10] Anyway, if we must rename it, exists_key is better than exists_at_key
[13:43:37] r: my %h=a=>1,b=>2; say %h.exists() # nobody worried about this silently failing ?
[13:43:40] <+camelia> rakudo 49f111: OUTPUT«False␤»

@lizmat
Copy link
Contributor Author

lizmat commented Jun 21, 2013

If .exists and .delete continue to exist as non-internal methods, then they should be specced.

@zhuomingliang
Copy link
Member

+1 to exists_key and delete_key

@pmichaud
Copy link
Contributor

If .exists and .delete continue to exist as non-internal methods,
then they should be specced.

http://perlcabal.org/syn/S32/Containers.html#exists
http://perlcabal.org/syn/S32/Containers.html#delete

Pm

@lizmat
Copy link
Contributor Author

lizmat commented Jun 21, 2013

There is no spec for .exists / .delete on hashes.

@pmichaud
Copy link
Contributor

I suspect the absence of .exists / .delete on hashes in S32 is more accidental than intentional, it could be added quickly enough.

I concur with jnthn and masak and others that .exists and .delete should remain, or at least not disappear for a long while. The implementation of adverbs was slushy and complicated enough (in both STD and Rakudo) that the method forms were the only mechanism available for programmers to use.

I find the analogy with .at_pos and .at_key to be somewhat telling. In some sense they're "internal methods", but they're not internal in the sense that they're really the preferred way for someone to implement a new indexable aggregate type. Properly overloading .[] or .{} for user-defined aggregates proved to be too unwieldy to do. I suspect that .exists and .delete fall in this category of "internal but visible" as well (perhaps as _key and _pos variants, as suggested).

I agree with TimToady that the method forms may need some changes to deal with LoL arguments... but I think this is another area where spec will have to follow implementation, not precede it.

Pm

@lizmat
Copy link
Contributor Author

lizmat commented Jun 21, 2013

Then I propose I spec .exists and .delete to always only be able to handle a single key, and that they're mainly intended for internal use only. For both arrays and hashes. And finish this discussion once and for all.

@lizmat
Copy link
Contributor Author

lizmat commented Jun 24, 2013

Specced Hash.exists and Hash.delete in 9b6b82a

@lizmat lizmat closed this as completed Jun 24, 2013
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

No branches or pull requests

3 participants