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

Proposed minor improvements #53

Merged
merged 1 commit into from Nov 4, 2019

Conversation

@manwar
Copy link
Contributor

manwar commented Nov 1, 2019

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

This comment has been minimized.

Copy link

coveralls commented Nov 1, 2019

Coverage Status

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

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Nov 1, 2019

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

This comment has been minimized.

Copy link
Contributor Author

manwar commented Nov 1, 2019

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

@oalders

This comment has been minimized.

Copy link
Member

oalders commented Nov 1, 2019

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

@karenetheridge

This comment has been minimized.

Copy link
Member

karenetheridge commented Nov 1, 2019

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 manwar force-pushed the manwar:proposed-minor-improvements branch from b32f09c to 918cf43 Nov 1, 2019
@manwar

This comment has been minimized.

Copy link
Contributor Author

manwar commented Nov 1, 2019

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

@oalders
oalders approved these changes Nov 2, 2019
@oalders

This comment has been minimized.

Copy link
Member

oalders commented Nov 4, 2019

Thanks @manwar!

@oalders oalders merged commit d1daa33 into libwww-perl:master Nov 4, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 77.419%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.