Moose
Moose copied to clipboard
moose
Make error messages for setting with a reader more awesome
Several times I've been bitten by Moose telling me that I'm using a read only accessor even when the attribute is 'rw' because something has set a custom writer. The error messages aren't awesome.
With these changes when attempting to erroneously set an attribute with a read-only accessor the Moose::Exception::CannotAssignValueToReadOnlyAccessor error raised now is more informative if a writer accessor exists, suggesting that that writer be used and supplying the name of the writer if it is non private (i.e. doesn't start with an underscore).
Looks good to me. I think I'd prefer dropping the "did you mean the private writer?" in the case of a private writer. I can see arguments both ways, though, and defer to other committer(s).
Anybody?
My general inclination is that we shouldn't mention a private writer, but I don't feel particularly strongly about that.
Otherwise, +1
16:43 @mst I think ... ah, I see what was bugging me 16:43 @mst the 'leaking information' argument 16:44 @mst so, "this attribute is read only and will not change" 16:44 @mst is not the same thing as "this method is read only but the attribute may change" 16:45 @mst and I think letting the user know 'the attribute may change' is not actually a leakage, since whether that method always returns the same value is part of the interface 16:45 @rjbs mst: I see, the distinction between "read-only accessor" and "read only attribute" being easy to look past when reading the error? 16:46 @rjbs Oh, I don't mind letting the user know that the attribute may change, but that could happen through many means. 16:47 @mst I feel like telling them there's a private writer is just an attempt to flail towards letting them know -that- and accidentally mistaking the most common implementation case for the general case
So, basically, you probably want to check e.g. for 'accessor' as well - but then you just need to say "read-only method (attribute is not read-only and may change)" or something. Probably also worth having that sort of message if there's a 'clearer'. So instead of 'there is a private writer', we're just saying 'this attribute's value may end up changing', and then going figuring how to change it is an excercise for the reader still but at least they know it's possible.
Here's my 2c:
I don't see the harm in leaking the information that the private writer exists given that there are several other ways that someone who is prepared to violate encapsulation is already able to do so without the information supplied. I find providing the fact a writer exists a helpful hint to the person working on the module (which may be the original author months later, or someone new now maintaining it), but not providing the name sufficient so that someone who is not intimate with the source code isn't encouraged to call it from outside the internals of the module itself.
Other people's opinions may differ, and I respect that, but that's my stance.
I didn't merge this for 2.1501-TRIAL, but I'm happy to do another release soon. Did we come to a consensus as to the behaviour for public and private writers?
No, I got busy on Friday. I was planning to take a look tomorrow.
The consensus reached on public writers was this was a good idea.
The suggestion that we're working on for private writers was something along the lines of 'it may be possible to mutate state by other means'. A hint that may or may not be present depending if Moose is aware of mutable state existing or not (moose can't be exhaustive on this because meta programming meets the halting problem - with great extensibility comes endless possibilities.). Anyway, the idea was possibly to emit the same suggestion on a clearer existence too.
Anyway, this is a long was of saying: no consensus yet, I'm going to work up a new patch with these ideas and we can start the debate all over again ;-)
On Sunday, July 19, 2015, Karen Etheridge [email protected] wrote:
I didn't merge this for 2.1501-TRIAL, but I'm happy to do another release soon. Did we come to a consensus as to the behaviour for public and private writers?
— Reply to this email directly or view it on GitHub https://github.com/moose/Moose/pull/106#issuecomment-122733705.
FWIW it's bad form to create a pull request from your master branch, as the entire pull request will be wiped out and closed if you ever try to re-use master for anything else (e.g. tracking the mainline master). Keeping the PR in a separate branch and not re-using it is best.
Where are we at with this? @2shortplanks did you have ideas for modifications?