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

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

Merged
merged 1 commit into from
Oct 17, 2018

Conversation

mschae94
Copy link

closes #214

@oalders
Copy link
Member

oalders commented Oct 16, 2018

Thanks @mschae94!

@oalders
Copy link
Member

oalders commented Oct 16, 2018

@petdance
Copy link
Contributor

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

@skaji
Copy link
Member

skaji commented Oct 16, 2018

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

@oalders
Copy link
Member

oalders commented Oct 17, 2018

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
Copy link
Member

oalders commented Oct 17, 2018

@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
Copy link
Contributor

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
Copy link
Member

skaji commented Oct 17, 2018

@oalders

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

I see.

@oalders
Copy link
Member

oalders commented Oct 17, 2018

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

oalders commented Oct 17, 2018

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

@oalders oalders merged commit c801a2d into libwww-perl:master Oct 17, 2018
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

Successfully merging this pull request may close these issues.

Please add clear_history method
5 participants