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

Make error messages for setting with a reader more awesome #106

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Make error messages for setting with a reader more awesome #106

wants to merge 6 commits into from

Conversation

2shortplanks
Copy link
Contributor

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).

If you try and set an attribute with the reader accessor and a different named writer accessor exists the error message now tells you the name of the writer
Change the changes to reflect that private accessors are no longer named
@rjbs
Copy link
Contributor

rjbs commented Jul 16, 2015

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?

@haarg
Copy link
Member

haarg commented Jul 16, 2015

My general inclination is that we shouldn't mention a private writer, but I don't feel particularly strongly about that.

Otherwise, +1

@shadowcat-mst
Copy link
Contributor

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.

@2shortplanks
Copy link
Contributor Author

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.

@karenetheridge
Copy link
Member

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?

@2shortplanks
Copy link
Contributor Author

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
#106 (comment).

@karenetheridge
Copy link
Member

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.

@karenetheridge
Copy link
Member

Where are we at with this? @2shortplanks did you have ideas for modifications?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants