POSIX is always present #4

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@yanick

yanick commented Mar 3, 2017

and it's used too at line 13, so the check afterward is unnecessary

POSIX is always present
and it's `use`d too, so the check afterward is
unnecessary
@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Mar 6, 2017

Owner

Hi @yanick

Thanks for the PR, appreciate your time.
I agree with you completely, since its already use'd then no point checking afterwards.

In my opinion, the use'd statement was a mistake and can go. No need to make any changes other that that, what do you think?

Many Thanks.
Best Regards,
Mohammad S Anwar

Owner

manwar commented Mar 6, 2017

Hi @yanick

Thanks for the PR, appreciate your time.
I agree with you completely, since its already use'd then no point checking afterwards.

In my opinion, the use'd statement was a mistake and can go. No need to make any changes other that that, what do you think?

Many Thanks.
Best Regards,
Mohammad S Anwar

@yanick

This comment has been minimized.

Show comment
Hide comment
@yanick

yanick Jun 19, 2017

POSIX seems to be needed for line 476 in the PR, so I think it has to stay. If this what you meant...?

yanick commented Jun 19, 2017

POSIX seems to be needed for line 476 in the PR, so I think it has to stay. If this what you meant...?

@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Jun 19, 2017

Owner

What I meant is the line 13, "use POSIX;" is not required.

If we remove the line 13 then everything else would make sense.

 17: my $can_locale;
 18: my $can_posix;
 19: BEGIN
 20: {
 21:      eval
 22:      {
 23:        $can_posix = 0;
 24:        require POSIX;
 25:        $can_posix = 1;
 26:     };
 ....
 ....
 ....
 483: $current_locale = $can_posix?  POSIX::setlocale(POSIX::LC_TIME())  :  q{};
Owner

manwar commented Jun 19, 2017

What I meant is the line 13, "use POSIX;" is not required.

If we remove the line 13 then everything else would make sense.

 17: my $can_locale;
 18: my $can_posix;
 19: BEGIN
 20: {
 21:      eval
 22:      {
 23:        $can_posix = 0;
 24:        require POSIX;
 25:        $can_posix = 1;
 26:     };
 ....
 ....
 ....
 483: $current_locale = $can_posix?  POSIX::setlocale(POSIX::LC_TIME())  :  q{};
@yanick

This comment has been minimized.

Show comment
Hide comment
@yanick

yanick Jun 19, 2017

Aaaaah, gotcha. Yes, that makes total sense. :-)

yanick commented Jun 19, 2017

Aaaaah, gotcha. Yes, that makes total sense. :-)

@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Jun 19, 2017

Owner

I will release the fix tomorrow.

Owner

manwar commented Jun 19, 2017

I will release the fix tomorrow.

@yanick

This comment has been minimized.

Show comment
Hide comment
@yanick

yanick Jun 19, 2017

\o/ No rush, though. It's not breaking anything and the patch was part of the CPAN Challenge. :-)

yanick commented Jun 19, 2017

\o/ No rush, though. It's not breaking anything and the patch was part of the CPAN Challenge. :-)

manwar added a commit that referenced this pull request Jun 20, 2017

- Fixed issue raised PR #4, thanks @yanick.
- Updated github repo info in the Makefile.PL script.
- POSIX is no longer pre-reqs but nice to have.
- Added MANIFEST file to the distribution.
- Updated README file.
- Tidied up Changes file.
@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Jun 20, 2017

Owner

Changes applied manually and released. Thanks @yanick

Owner

manwar commented Jun 20, 2017

Changes applied manually and released. Thanks @yanick

@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Jun 24, 2017

Owner

Thanks for raising the issue @yanick. Since the fix is released I am closing the issue.

Owner

manwar commented Jun 24, 2017

Thanks for raising the issue @yanick. Since the fix is released I am closing the issue.

@manwar manwar closed this Jun 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment