Closed Simn closed 4 years ago
second hint (because check detection logic might be a bit complex): check detection calls check.configureProperty
which uses Reflect.setField
on checks under detection to run them with different settings.
The same configureProperty
is used after reading a checkstyle configuration file during a "normal" checkstyle run.
I just checked and this is still a problem.
I've changed one of the tests like so:
@Test
public function testDetectSeparatorWrap() {
var check = new SeparatorWrapCheck();
var detectedChecks:Array<CheckConfig> = DetectCodingStyle.detectCodingStyle([check], [buildCheckFile(SAMPLE_CODING_STYLE)]);
Assert.areEqual(1, detectedChecks.length);
Assert.areEqual("SeparatorWrap", detectedChecks[0].type);
var props = cast detectedChecks[0].props;
Sys.println(props); // {}
Sys.println(check.option); // eol
Sys.println(props.option); // null
Assert.areEqual(WrapCheckBaseOption.EOL, props.option);
}
So the expected value is on the original instance of SeperatorWrapCheck
, but the props
object is empty.
I've noticed that ConfigUtils.makeCheckConfig
uses Reflect.fields(check)
, which is not guaranteed to work for classes:
This method is only guaranteed to work on anonymous structures. Refer to `Type.getInstanceFields` for a function supporting class instances.
Yes the tests pass after changing that to for (prop in Type.getInstanceFields(Type.getClass(check))) {
. But then the non-genjvm tests fail with Cannot access field for writing
.
Anyway, genjvm is within the spec here, but to be honest I'm not sure why we can't just make Reflect.fields
call Type.getInstanceFields
if the passed object is a Haxe-class.
I guess I should have read the docs more carefully instead of going with what worked when writing that function.
getInstanceFields
works but an extra Reflect.isFunction
is needed to skip putting functions into the resulting check configuration.
fixed in https://github.com/HaxeCheckstyle/haxe-checkstyle/pull/498
note: I rearranged build files a bit, there is now a testJava.hxml
and a testJvm.hxml
Alright, thanks for the info! I also realized that I would have to add this isFunction
check if I wanted to call getInstanceFields
from Reflect.fields
. That seems pretty heavy, so I prefer to not do it and point to the documentation.
I would only consider this if all targets already worked like that, but I don't think that's the case. You probably just got lucky that it works on the targets you were using.
@AlexHaxe's hint: