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

Replace barewords with lexicals #23

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@paultcochrane
Contributor

paultcochrane commented Aug 15, 2018

One theoretically shouldn't use barewords in open or opendir commands anymore. This patch updates the code to use lexicals instead as required by current Perl best practices.

As usual with my patches, if you want something changed, just let me know and I'll update and resubmit :-)

open TCLSH, "$tclsh test-for-tk.tcl|";
my $res = join '', <TCLSH>;
open my $tk_test, "$tclsh test-for-tk.tcl|";
my $res = join '', <$tk_test>;

This comment has been minimized.

@chrstphrchvz

chrstphrchvz Aug 15, 2018

Contributor

I'm not requesting changes; I've just wondered if it would be alright to use my $res = qx/$tclsh test-for-tk.tcl/ instead for simplicity.

@chrstphrchvz

chrstphrchvz Aug 15, 2018

Contributor

I'm not requesting changes; I've just wondered if it would be alright to use my $res = qx/$tclsh test-for-tk.tcl/ instead for simplicity.

This comment has been minimized.

@paultcochrane

paultcochrane Aug 15, 2018

Contributor

That's good idea; i wanted to focus the first patch on just the barewords to keep it tight and easy to merge. The plan would be to then update the open statements to use the three-arg form where possible. In this case, the three-arg form would be unnecessary, since one could replace it with a qx{}, so thanks for mentioning this :-)

@paultcochrane

paultcochrane Aug 15, 2018

Contributor

That's good idea; i wanted to focus the first patch on just the barewords to keep it tight and easy to merge. The plan would be to then update the open statements to use the three-arg form where possible. In this case, the three-arg form would be unnecessary, since one could replace it with a qx{}, so thanks for mentioning this :-)

@vadrer

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Aug 15, 2018

Owner

we don't use backticks `` in tcl.pm makefile.pl, because we check for return code, not only we read output
IOW I try to keep logic of these two makefile.pl-s similar, this is why 2 lines of code instead of 1

next, I myself use open my $fh, but right here I don't, because this introduces dependency on 5.6

Owner

vadrer commented Aug 15, 2018

we don't use backticks `` in tcl.pm makefile.pl, because we check for return code, not only we read output
IOW I try to keep logic of these two makefile.pl-s similar, this is why 2 lines of code instead of 1

next, I myself use open my $fh, but right here I don't, because this introduces dependency on 5.6

@chrstphrchvz

This comment has been minimized.

Show comment
Hide comment
@chrstphrchvz

chrstphrchvz Aug 15, 2018

Contributor

Hmm, I thought backticks updates $?—does it do so differently than open()?

Contributor

chrstphrchvz commented Aug 15, 2018

Hmm, I thought backticks updates $?—does it do so differently than open()?

@vadrer

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Aug 15, 2018

Owner

indeed.

go for ``

Owner

vadrer commented Aug 15, 2018

indeed.

go for ``

@chrstphrchvz

This comment has been minimized.

Show comment
Hide comment
@chrstphrchvz

chrstphrchvz Aug 15, 2018

Contributor

Ahh, now I see in tcl.pm Makefile.PL the open … or die …—that could be a case where backticks doesn't necessarily simplify things (might require a separate die statement).

Contributor

chrstphrchvz commented Aug 15, 2018

Ahh, now I see in tcl.pm Makefile.PL the open … or die …—that could be a case where backticks doesn't necessarily simplify things (might require a separate die statement).

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Aug 15, 2018

Contributor

How about I close this PR as unmerged (since the change doesn't work for Perls under 5.6) and try a new patch to simplify the open call?

Contributor

paultcochrane commented Aug 15, 2018

How about I close this PR as unmerged (since the change doesn't work for Perls under 5.6) and try a new patch to simplify the open call?

@vadrer

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Aug 15, 2018

Owner

yes, that would be nice :)

Owner

vadrer commented Aug 15, 2018

yes, that would be nice :)

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