search

libwww-perl / WWW-Mechanize

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

Pass strict/verbose constructor args to HTML::Form #256

Closed simbabque closed 6 years ago

simbabque commented 6 years ago

Closes #255

oalders commented 6 years ago

Thanks so much for this. Should we add a test that demonstrates that strict_forms can be disabled on a case by case basis when calling submit_form?

oalders commented 6 years ago

Also, it would be great to document this new behaviour in the best practices section as well as the SYNOPSIS.

simbabque commented 6 years ago

I think I have this actually.

oalders commented 6 years ago

I was looking specifically for strict_forms => 0 in a submit_form call in the test, but I don't see it.

simbabque commented 6 years ago

Line 228 in the new test file should have been 0, my bad. The name of the text is correct. I'll fix this tomorrow. At the London Perl tech meeting now.

oalders commented 6 years ago

Thanks! Enjoy your meeting. :)

simbabque commented 6 years ago

Should we add a test that demonstrates that strict_forms can be disabled on a case by case basis when calling submit_form?

I've updated my commit to fix the test.

Also, it would be great to document this new behaviour in the best practices section as well as the SYNOPSIS.

I've done the best practices, but I am not sure about the SYNOPSIS.

From the amount of questions I've answered about Mech on Stack Overflow I believe a lot of new users start by copying the SYNOPSIS code without really understanding what's going on there. For them, the strict forms option would not really make their code more robust, but just confuse them more because they will likely be confronted with even more errors they don't understand.

On the other hand, it helps to keep these new users on track. I've added it as a separate commit so we can cherry-pick it out if needed.

I do not think we should be talking about verbose mode a lot. Catching these warnings is quite an advanced topic. Having it documented as an option is probably enough for people who know what they are doing.

petdance commented 6 years ago

I think it would be good if the docs explained what the strict/verbose flags do when passed through to HTML::Form. It doesn't need to replicate the docs, but a one-sentence summary would be helpful to the reader.

And is verbose not a best practice?

simbabque commented 6 years ago

I think it would be good if the docs explained what the strict/verbose flags do when passed through to HTML::Form. It doesn't need to replicate the docs, but a one-sentence summary would be helpful to the reader.

I included that. Please see https://github.com/libwww-perl/WWW-Mechanize/pull/256/files#diff-9b033b946ad0ade000c98612745f570eR248.

And is verbose not a best practice?

I guess it should be, but it's not the default in HTML::Form and I believe we should not just turn on a new feature that will introduce fatal errors where none were there before. That would break a lot of users' code.

petdance commented 6 years ago

I included that.

Sorry, my mistake. That looks good.

As to verbose being a best practice, I didn't think that listing it in "best practices" would include making it be the default. We'd just be specifying that it's a good idea, right?

Note that I haven't used either of these features myself, but will be eager to play with them when they go live.

simbabque commented 6 years ago

As to verbose being a best practice, I didn't think that listing it in "best practices" would include making it be the default. We'd just be specifying that it's a good idea, right?

Oh, I misunderstood. Sorry about that. However, was already in the best practices section. I've extended it in my commit. Please see https://github.com/libwww-perl/WWW-Mechanize/pull/256/files#diff-9b033b946ad0ade000c98612745f570eR3219.

petdance commented 6 years ago

Is there anything you'd like me to do to test this out?

simbabque commented 6 years ago

If you're asking me, feel free to play with it. I wrote unit tests, but I didn't actually use it. I don't use Mech a lot lately as my focus at $work has shifted. At the moment I'm doing these tickets for fun and for the sake of doing them. :)

petdance commented 6 years ago

I'd like to suggest/request that when all this is churned over that a new PR gets made with all the changes and feedback. It's difficult to look over multiple commits on something like this.

simbabque commented 6 years ago

I'd like to suggest/request that when all this is churned over that a new PR gets made with all the changes and feedback. It's difficult to look over multiple commits on something like this.

I agree. When everyone's happy, I'll squash down the branch and create a single-commit new PR.

petdance commented 6 years ago

One other request: I'd like it if we used proper capitalization and punctuation in comments, so that instead of:

# enable strict form processing to catch typos and non-existant form fields

we have

# Enable strict form processing to catch typos and non-existent form fields.

Makes it easier to read. (Also, it's "non-existent" not "non-existant".)

petdance commented 6 years ago

Is there a timeframe for making a release of WWW::Mechanize with this PR in it? I'm going to put out a release of Test::WWW::Mechanize soon, and I'd like to mention in it that the underlying WWW::Mechanize has new features in it that may be useful to testers.

oalders commented 6 years ago

I can release this on the same day as this gets merged. I'm happy with the current state of things. The code comments could be cleaned up here or in a different merge. I'm not picky about that.

petdance commented 6 years ago

I just ran a small subset of our test suite using this Mech branch and the new options enabled and already turned up a couple of problems.

strict_forms caused an error of "Input 'sb_selectedoption' is readonly at /home/alester/...."

verbose_forms threw a warning saying " outside

", which combined with Test::Warnings, made my test fail (good!).

👍 to this level of pickiness. I'll start testing in earnest once it hits the CPAN.

simbabque commented 6 years ago

I'd like it if we used proper capitalization and punctuation in comments

I don't want to nitpick, but it seems that actually most of the code base has single sentence comments that start with a capital letter, but do not have a full stop. There are only full stops if the comment has more than one sentence. However, this is in the synopsis. So it's not even a real comment.

I am all for consistency, so I'll change that.

@oalders, do you want me to merge them down and create a new PR or are we OK with a bit of history going in?

oalders commented 6 years ago

@simbabque in the interest of getting this done, I'm going to merge what we have here and get a release out there. I think it looks good.

oalders commented 6 years ago

Thanks @simbabque, @petdance and @PatrickCronin!

oalders commented 6 years ago

New version is now on CPAN.