search

libwww-perl / LWP-Protocol-https

Provide https support for LWP::UserAgent
https://metacpan.org/pod/LWP::Protocol::https
Other
16 stars 35 forks source link

Add defined check #53

Closed manwar closed 4 years ago

manwar commented 4 years ago

Hi @oalders

Please review the PR. This was assigned to me as November month assignment by Pull Request Club.

Many Thanks. Best Regards, Mohammad S Anwar

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 77.419% when pulling 918cf43c9b87fb9b9a1c2d1d32f6ea5a374e4e6e on manwar:proposed-minor-improvements into 14df39e82514a6b97161421434294a450fcafc7b on libwww-perl:master.

oalders commented 4 years ago

Thanks @manwar. I think I understand what you're doing, but it's not clear to my why you're using our rather than my here. Having said that, I wonder if we'd be better off using something like Const::Fast here, if we were ok with the added dependencies.

manwar commented 4 years ago

@oalders there is no particular reason of using "our". I am happy to change to whatever you suggest to.

oalders commented 4 years ago

Ok, I've asked for others to comment on this as well, so maybe hold off before making any changes. :)

karenetheridge commented 4 years ago

I'm not a fan of changing the use of these strings -- it adds cognitive load to have to keep referring elsewhere in the document to see what these values are.

The defined check is great.

manwar commented 4 years ago

@karenetheridge I have removed the changes completely and kept just defined check. Is this any good?

oalders commented 4 years ago

Thanks @manwar!