Extract runtime detection constants into separated module

[happy-barney]

Runtime detection code was implemented in multiple sources many times using same logic.

Extracting such logic into module and import required behaviour as symbols improves reliability of source code (using symbols instead of constants, write logic only once) as well as (usually) shows some improvements possibilities.

[Leont]

I think adding the platform definitions make sense (though I'm not sure Runtime is the right name, maybe TAP::Harness::Constants? TAP::Harness::Platform?). I think the HAS_TIME_HIRES part is wrong.

Firstly, it looks to me like the Time::HiRes logic is kind of failing to actually make things simpler than they were before.

Secondly, Time::HiRes has been in core since 5.8.0; instead of improving this logic we may be able to remove the eval logic entirely (though I would need to double-check if core needs a pure-perl mode).

[Leont]

Also, the conditional loading of time logic is wrong, and will blow up if Time::HiRes doesn't load. eval { use } won't do what you want because the use is run at compile time and the eval only affects the runtime.

[happy-barney]

I think adding the platform definitions make sense (though I'm not sure Runtime is the right name, maybe TAP::Harness::Constants? TAP::Harness::Platform?). I think the HAS_TIME_HIRES part is wrong.

Firstly, it looks to me like the Time::HiRes logic is kind of failing to actually make things simpler than they were before.

8a44bd2 - forgotten quoting

Secondly, Time::HiRes has been in core since 5.8.0; instead of improving this logic we may be able to remove the eval logic entirely (though I would need to double-check if core needs a pure-perl mode).

this PR is only about moving things around ... I'll update PR message

[Leont]

this PR is only about moving things around ... I'll update PR message

The point of refactoring is not to make the code prettier, it's to make it more maintainable. This part of the proposed change is not doing that at all for me.

[happy-barney]

I don't understand what you meant. Can you explain?

Removing duplicated code is always improving maintainability.
Using symbol to express intention instead of implementing something - same.

[karenetheridge]

This changes the eval from a block style evaluation to a string, which is not a good change.

[Grinnz]

Block eval would not be able to test a use statement, but this can easily be done with a block eval: eval { require Time::HiRes; Time::HiRes->import('time'); 1 }

[happy-barney]

@karenetheridge original code had string evaluation.

I'm using form q { } for string containing code (hence { as paired delimiter).

commit introducing this change is fixup! of original commit. I'm using such approach to show reaction to code review comment, referring fixup commit(s) dealing with given issue raised in given comment.
(when I was taught this approach I hesitated but at the end, in many PR-review cycles, I found out that review by commit and reacting with fixups saved lot of time especially on reviewer's side)

@Grinnz my aim was only to extract existing runtime detection into dedicated library, but your suggestion is too good to be omitted ... 033f677

[Leont]

Secondly, Time::HiRes has been in core since 5.8.0; instead of improving this logic we may be able to remove the eval logic entirely (though I would need to double-check if core needs a pure-perl mode).

I've done just that in #141

[Leont]

Removing duplicated code is always improving maintainability.

You're deduplicating 3 lines of code around Time::HiRes. If you need a multitude of those 3 lines (and in this case a custom importer) to do so you're not necessarily improving maintainability.

Using symbol to express intention instead of implementing something - same.

It's already doing that right now.

[karenetheridge]

I agree with @Leont -- moving these checks to a separate file does not enhance readability, but diminishes it. Defining the constants in the file where they are used makes more sense to me.

I also agree that the Time::HiRes usage can probably be improved, but moving it to another file doesn't do that.

[happy-barney]

I agree with @Leont -- moving these checks to a separate file does not enhance readability, but diminishes it. Defining the constants in the file where they are used makes more sense to me.

As I perceive this: it's about encapsulation, about expressing intention vs implementing behaviour.

Importing well named symbol is same way as declaring such symbol on your own, but fixing symbol is much easier than
fixing tons of implementation. As I learned during writing this PR, $^O is win32 even on 64-bit windows - just because there is too many win32 constants around CPAN. Should there be symbol IS_WINDOWS since beginning, no one will care about implementation details.

Some stats from CPAN (using metacpan/metacpan-cpan-extracted)

$ ack -h -i '\$\^O.*win32' | wc -l
25539

$ ack -h '\$\^O.*VMS' | wc -l
6929

$ ack -h '\$\^O' | wc -l
50009

Without Time::HiRes, this Runtime package should be distributed as standalone package (even implemented as builtin in future), eg:

package Perl::Platform;

use constant IS_VMS ...;
use constant IS_OPENBSD ...;
use constant IS_WINDOWS ...;
use constant IS_CYGWIN ...;
...


> 
> I also agree that the Time::HiRes usage can probably be improved, but moving it to another file doesn't do that.

There is an issue #141 for that now, so let me show you my approach to refactoring, next PR will be dealing with that.