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

Document current best practices #314

Merged
merged 3 commits into from
Apr 10, 2019
Merged

Document current best practices #314

merged 3 commits into from
Apr 10, 2019

Conversation

oalders
Copy link
Member

@oalders oalders commented Apr 10, 2019

No description provided.

@oalders oalders requested a review from genio April 10, 2019 15:30
lib/LWP/UserAgent.pm Outdated Show resolved Hide resolved
@genio
Copy link
Member

genio commented Apr 10, 2019

I'm 👍 on this idea

die $response->status_line;
}
my $jar = HTTP::CookieJar::LWP->new;
my $agent = LWP::UserAgent->new(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/$agent/$ua/

Also, I think we should use the same variable name for LWP::UserAgent objects in this document.
So why don't you use $ua in "BEST PRACTICES" section too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've fixed all of those cases now. This was partly a copy/paste from WWW::Mechanize, which is where $agent came from. Although, looking more closely at that, it should probably have been $mech in the WWW::Mechanize docs, since that's what everything else there uses.

my $response = $ua->get('http://example.com');

if ($response->is_success) {
print $response->decoded_content; # or whatever
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can do without the # or whatever comment

I usually use the ... to indicate arbitrary code that can be anything before or after ... or here, in the middle

But having the decoded_content is a good example too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I'm not sure why someone cough cough ever put that in there. It reads oddly passive aggressive. /me hides

lib/LWP/UserAgent.pm Outdated Show resolved Hide resolved
skaji
skaji previously approved these changes Apr 10, 2019
Copy link
Member

@skaji skaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@coveralls
Copy link

coveralls commented Apr 10, 2019

Coverage Status

Coverage remained the same at 60.411% when pulling 96d7360 on oalders/best-practices into 28f8055 on master.

@genio genio merged commit 14f0a97 into master Apr 10, 2019
@genio genio deleted the oalders/best-practices branch April 10, 2019 21:45
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.

None yet

5 participants