Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Github Issue:1178 -- throw warning with trying to set readonly attribute #1181

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants

comjf commented Oct 26, 2013

Inspired from this issue: jnicklas#1178

This is my first PR to an open source project, so please correct me if I didn't follow some protocol, I'd like to contribute more in the future.

I decided to throw a warning instead of throwing an error (As the issue requested) for backwards compatibility

I did not create any new tests, but modified the 2 tests I could find that cared about read only fields.

Collaborator

jnicklas commented Oct 28, 2013

Thanks for contributing, and welcome ;)

I'm not sure if I like using a warning like this. At least the warning could be more descriptive.

I'd actually forgotten that we'd written tests codifying the behaviour of ignoring writes to readonly elements. Semver then tells us we shouldn't raise an exception.

I'm torn on this one. @twalpole, what do you think?

comjf commented Oct 28, 2013

Thanks! I can obviously change the warning to be whatever message you guys like, I just wanted to get start contributing to see if I could do it.

Is there a priority list or something for my to look into doing next?

Collaborator

jnicklas commented Oct 28, 2013

@comjf not really. Just check around on the issues list if you see anything open that you think you can fix. Thanks for helping out!

Collaborator

twalpole commented Oct 28, 2013

I would lean more towards raising an exception (and bumping the version if we feel necessary) since users obviously can't edit readonly fields and therefore any test that attempts to do so is broken. This change could be reserved until we have a few other breaking changes that would justify a version bump. One solution may be to go with a deprecation warning for now and change to raising an exception at the next breaking version.

Collaborator

jnicklas commented Nov 26, 2013

So what do we do about this? I'm not super enthusiastic about forcing drivers to print a warning to pass the suite, I don't think that's the right call.

comjf commented Nov 26, 2013

Sorry, I completely forgot about this. I'll go ahead and update to have it raise an exception later this week.

What would you like the exception to be?

Thanks for the reminder.

Collaborator

twalpole commented Nov 26, 2013

@jnicklas - how about we have the built-in drivers output a deprecation warning for now, but not test for it - and add a test for throwing an exception that is pending until version >= 3.0.0 which would be when semver allows for the change

Collaborator

jnicklas commented Nov 26, 2013

@twalpole that sounds like an excellent suggestion. 👍

Collaborator

abotalov commented Feb 5, 2014

@comjf Do you plan to work on this? Otherwise I can do it myself.

Collaborator

twalpole commented Apr 2, 2014

@comjf / @abotalov - any progress on this?

comjf commented Apr 3, 2014

@twalpole Please review, I'm rusty and completely forgot about this. I didn't quite understand what you meant by 'have the built-in drivers output a deprecation warning for now', not sure how to do that. So I simply made a new exception and modified tests for it. When 3.0 is ready we can look at this again.

Perhaps I should close this and make a new PR to a different branch? (I also need to squash my history)

Collaborator

twalpole commented Apr 3, 2014

@comjf The thought was that instead of raising an exception currently, just do

warn "Attempt to set readonly element with value: #{value}"  

but add a test that checks for an exception being thrown if the version of capybara is >= 3 which will remind us that we need to implement that in the future. Don't test for the warn though since it's not actually an update to the required API for other drivers

comjf commented Apr 3, 2014

Ah ok, that should be easy enough, sorry that I misunderstood you. I'll close this and reopen a PR with just what you describe, but is there anything else that should be done at the moment?

Thanks,
James Flowers

Collaborator

twalpole commented Apr 3, 2014

nope -- I think those changes would complete this - thanks

@abotalov abotalov added the Bug label Apr 4, 2014

@comjf comjf closed this Apr 7, 2014

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