search

libwww-perl / WWW-Mechanize

Handy web browsing in a Perl object
https://metacpan.org/pod/WWW::Mechanize
Other
68 stars 52 forks source link

Issue 214: add ability to clear history and tests for history #259

Closed mschae94 closed 5 years ago

mschae94 commented 5 years ago

closes #214

oalders commented 5 years ago

Thanks @mschae94!

oalders commented 5 years ago

@petdance this would close a pretty old issue: https://web.archive.org/web/20130622184440/http://code.google.com/p/www-mechanize/issues/detail?id=237

petdance commented 5 years ago

Sounds good to me. Are you wanting me to do something?

skaji commented 5 years ago

Do we need to clear other variables such as $self->{req}, $self->{res} too?

oalders commented 5 years ago

Do we need to clear other variables such as $self->{req}, $self->{res} too?

I think since this is just deleting the stack, we want to keep the information for the last request around. https://metacpan.org/source/OALDERS/WWW-Mechanize-1.88/lib/WWW/Mechanize.pm#L206-218

oalders commented 5 years ago

@petdance if you had a moment to eyeball the code, that would be helpful. I didn't see any issues, but more eyes on the code is better.

petdance commented 5 years ago

I think $self->{page_stack} should get reinitialized to an empty [], just like in new, after it gets deleted.

Are we still supporting Perls all the way back to 5.6.1? Even on ack I require 5.10.1.

The test seems to be mixing Test and Test2 style matches. I'm seeing both isa and isa_ok.

That's all I'm seeing. All seems sensible to me.

skaji commented 5 years ago

@oalders

I think since this is just deleting the stack, we want to keep the information for the last request around.

I see.

oalders commented 5 years ago

Are we still supporting Perls all the way back to 5.6.1? Even on ack I require 5.10.1.

We've been keeping stuff backwards compatible as far as possible unless there's a compelling reason not to.

I'm seeing both isa and isa_ok.

Yeah, I'm not sure isa_ok() is providing much benefit. We could probably delete those checks.

That's all I'm seeing. All seems sensible to me.

Thanks for the review @petdance!

mschae94 commented 5 years ago

The test seems to be mixing Test and Test2 style matches. I'm seeing both isa and isa_ok.

Actually, for the isa_ok, I have copied some codes from the existing tests for the consistency purpose. For the isa, this is one of the special comparisons provided by Test::Deep.

I think $self->{page_stack} should get reinitialized to an empty [], just like in new, after it gets deleted.

In _push_page_stack it reinitializes to an empty [] if $self->{page_stack} does not exist. Therefore, we can simply delete the key i guess.

simbabque commented 5 years ago

For what it's worth, I don't see any issues with the changes either. But I'm just a passer-by and I've helped @mschae94 implement them. :)

oalders commented 5 years ago

Thanks @mschae94, @petdance, @skaji and @simbabque!