Fix #457 - Allow to apply checks only for specific modules

[wilzbach]

This (#457) would be very helpful for Phobos as it allows to introduce new checks gradually over time.
I discovered a dinifiled bug while developing this (the last variable always overwrites the according field in the main struct). However, as an easy workaround for now one can simply add a dummy variable at the end of the .ini file.
I think my idea is best explained by copy/pasting the the README section:

Selecting modules for a specific check

It is possible to create a new section analysis.config.ModuleFilters in the .dscanner.ini.
In this optional section a comma-separated list of inclusion and exclusion selectors can
be specified for every check on which selective filtering should be applied.
These given selectors match on the module name and partial matches (std. or .foo.) are possible.
Morover, every selectors must begin with either + (inclusion) or - (exclusion).
Exclusion selectors take precedence over all inclusion operators.
Of course, for every check a different selector set can given:

[analysis.config.ModuleFilters]
final_attribute_check = "+std.foo,+std.bar"
useless_initializer = "-std."

A few examples:

• +std.: Includes all modules matching std.
• +std.bitmanip,+std.json: Applies the check only for these two modules
• -std.bitmanip,-std.json: Applies the check for all modules, but these two
• +.bar: Includes all modules matching .bar (e.g. foo.bar, a.b.c.barros)
• -etc.: Excludes all modules from .etc
• +std,-std.internal: Includes entire std, except for the internal modules

Compatibility

I tried to develop this with backwards compatibility in mind as e.g. the following

[analysis.config.Checks.final_attribute_check]
enabled="yes"
skip_tests="yes"
inclusion="foo,bar"
exclusion="barros

With the proposed approach Dscanner will continue to work with existing configs and if users want to use this new feature, they can simply add a [analysis.config.ModuleFilters] section to their config file.

Enjoy!

[wilzbach]

I have pushed this branch to dlang-community/phobos, because as mentioned this feature is very useful for Phobos:

dlang/phobos#5495

If this gets merged into master, we could delete the "phobos" branch - though it might not be too bad to have a special branch for Phobos and thus being able to test features or apply workarounds without affecting the Dscanner.

[ghost]

Can't you report and/or fix the problem in inifiled ? The library is quite small, should be doable.

[wilzbach]

Can't you report and/or fix the problem in inifiled ? The library is quite small, should be doable.

Yes, will do - just didn't had the time to reduce the bug and fix it when I was working on this PR.

[wilzbach]

Can't you report and/or fix the problem in inifiled ? The library is quite small, should be doable.

burner/inifiled#9

[wilzbach]

Can't you report and/or fix the problem in inifiled ? The library is quite small, should be doable.
burner/inifiled#9

Thanks to @burner's fast feedback, merge & tagging, I could remove the workaround with the pseudo-variable and updated inifiled to 1.0.2 :)

[wilzbach]

Huh? How is it possible that we have run inifiled's testsuite all the time without noticing?

./bin/dscanner-unittest
std.exception.ErrnoException@std/stdio.d(403): Cannot open file `test/filename.ini' in mode `r' (No such file or directory)
----------------
??:? @safe shared(core.stdc.stdio._IO_FILE)* std.exception.__T12errnoEnforceTPOS4core4stdc5stdio8_IO_FILEVAyaa11_7374642f737464696f2e64Vmi403Z.errnoEnforce(shared(core.stdc.stdio._IO_FILE)*, lazy immutable(char)[]) [0x1080535]
??:? ref @safe std.stdio.File std.stdio.File.__ctor(immutable(char)[], const(char[])) [0x104a158]
inifiled/source/inifiled.d:64 void inifiled.readINIFile!(inifiled.Person).readINIFile(ref inifiled.Person, immutable(char)[]) [0xe6a04f]
inifiled/source/inifiled.d:520 void inifiled.__unittestL497_106() [0xe695d7]
??:? void inifiled.__modtest() [0xe6d2b0]

AFAICT the only forward is to fix the makefile and build the libraries as static library separately.
This also has the additional benefit that building the testsuite will be faster :)

[dlang-bot]

Thanks for your pull request, @wilzbach!

[wilzbach]

Huh? How is it possible that we have run inifiled's testsuite all the time without noticing?

The answer is simple: the previously checked-in version of inifiled had an external testsuite: https://github.com/burner/inifiled/blob/73377121a98edf1da0152a54c0b24968792f51d9/source/inifiled.d (no inline unittest).

Btw anything blocking this?
This feature is already used successfully at Phobos: https://github.com/dlang/phobos/blob/master/.dscanner.ini#L105

[ghost]

I see that you referenced it. Actually i've approved two days ago and you could have merged at this time !

[wilzbach]

I see that you referenced it. Actually i've approved two days ago and you could have merged at this time !

Thanks a lot. I didn't merge this because I prefer to avoid merging my own PRs and secondly because it's quite a non-trivial PR with some implications (changed test build, changed config file, ...).