add linting via dscanner #88

wilzbach · 2016-04-14T05:45:19Z

I think we should try putting the linting off to a real bot.
This PR adds a execution of dscanner.

We probably want to play with this for a while, maybe disable some checks or fix the codebase until we set the --enforce flag, which will exit the CI with an error.
Currently it outputs many messages, see e.g. here

Other

It is only run on travis subbuild - hence the version check (I reused it for the s3 check)
As there is no binary release of dscanner, I created releases.dlang.io (a s3 bucket to which I will upload such release) - this speeds up the build! Otherwise there is still dub fetch & dub run.

codecov-io · 2016-04-14T05:49:41Z

Current coverage is `98.17%`

Merging #88 into master will increase coverage by +0.05% as of ec51871

@@            master     #88   diff @@
======================================
  Files            7       7       
  Stmts         2500    2580    +80
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           2453    2533    +80
  Partial          0       0       
  Missed          47      47

Review entire Coverage Diff as of ec51871

Powered by Codecov. Updated on successful CI builds.

9il · 2016-04-14T05:56:57Z

I don't think dscanner is good enought

wilzbach · 2016-04-14T05:58:20Z

I don't think dscanner is good enought

AFAIK it's the best tool out there and we can just disable the warnings that we don't like.
In my opinion a linting is better than no linting?

9il · 2016-04-14T05:59:19Z

ping me when you would think PR is ready

wilzbach · 2016-04-14T14:14:48Z

@9il I think I am getting somewhere, but for the remaining issues I would like to hear your opinion and help. I do consider static analysis of code important as it can prevent a couple of errors and it ensures that the codes sticks to a style guide - the latter being the main reason for my attempt here.
Dscanner checks for a lot of stuff - have a look at the .dscanner.ini. It was really helpful to realize that the new combinatorics module didn't document itself properly.

You can run it locally with:

dub fetch dscanner --version=~master
dub run dscanner -- --config .dscanner.ini --styleCheck source

Or install the dscanner-git package on Arch Linux. I recommend storing the binary somewhere as you avoid the dub checking and can pass the spammy error log to dev/null. I use

dscanner --config .dscanner.ini --styleCheck source 2> /dev/null

I grouped them after their warning types, so that it's easier to look at them.
Please do consider that disabling a check because of one or two edge cases results in we no benefit from this help in the future. I still didn't set the --enforce flag, but I plan to do so after we resolved remaining the conflicts below.

Naming style guide

From Phobos: functions + variables in camelCase.

source/mir/las/sum.d(2125:11)[warn]: Function name 'SummationAlgo' does not match style guidelines.
source/mir/las/sum.d(2135:11)[warn]: Function name 'SummationAlgo' does not match style guidelines.
source/mir/ndslice/internal.d(174:21)[warn]: Variable name 'DynamicArrayDimensionsCount' does not match style guidelines.
source/mir/ndslice/internal.d(176:21)[warn]: Variable name 'DynamicArrayDimensionsCount' does not match style guidelines.
source/mir/ndslice/internal.d(256:8)[warn]: Struct name '_Slice' does not match style guidelines.
source/mir/ndslice/slice.d(1042:21)[warn]: Variable name 'PureN' does not match style guidelines.

Subtracting from `.length`

source/mir/ndslice/selection.d(221:29)[warn]: Avoid subtracting from '.length' as it may be unsigned.
source/mir/ndslice/selection.d(222:29)[warn]: Avoid subtracting from '.length' as it may be unsigned.

Zero-parameter '@Property' function

source/mir/ndslice/selection.d(937:22)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
source/mir/ndslice/selection.d(979:22)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
source/mir/ndslice/selection.d(1422:22)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
source/mir/ndslice/slice.d(1327:14)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
source/mir/ndslice/slice.d(1374:14)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
source/mir/ndslice/slice.d(2845:14)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.

'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' should be const

source/mir/ndslice/slice.d(1591:10)[warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.
source/mir/ndslice/slice.d(1609:10)[warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.

has method 'opEquals', but not 'toHash'

source/mir/ndslice/slice.d(1027:8)[warn]: 'Slice' has method 'opEquals', but not 'toHash'.

Avoid use of the comma expression

source/mir/ndslice/selection.d(763:66)[warn]: Avoid using the comma expression.

Disabled flags

Moreover I disabled a couple of flags temporarily - I do plan to fix the codebase at a later point and toggle this on, but let's go step by step.

Here's the list with the reason:

undocumented public documentation: it correctly realizes that for /// the documentation is undefined. not sure yet whether this is a bug. Should we write /// ditto?
"Local imports should specify the symbols being imported to avoid hiding local symbols": there were too many violations.
unused variable: issue #326
could be immutable: issue #325
checks for labels with the same name as variables: issue #323

wilzbach · 2016-04-14T14:16:10Z

source/mir/combinatorics/package.d

+
+/// ditto
 struct IndexedRoR(Collection, Range)
 if (isInputRange!Range)


this didn't have docs ...

9il · 2016-04-14T14:20:38Z

Naming style guide

turn off please

wilzbach · 2016-04-14T14:21:21Z

source/mir/ndslice/selection.d


            static if (canSave!PureRange)
-            auto save() @property
+            auto save()


@property methods with zero arguments should return const.
Changing this as it still can be used via UCFS

9il · 2016-04-14T14:23:04Z

I would not merge this until the Phobos merged or you remove changes in ndslice package

9il · 2016-04-14T14:23:26Z

Please move all changes to separate PR (and without ndslice)

wilzbach · 2016-04-14T14:25:06Z

@9il I tried to describe why I did some of the changes - hope this helps to understand ;-)
Will separate now.

wilzbach · 2016-04-15T15:38:16Z

source/mir/combinatorics/package.d

    Forward range, which yields the permutations
+
+See_Also:
+    $(LREF Permutations), $(LREF dispose)


As it is bundled with makePermutations, linking to dispose could be useful to the user

dispose make refer to another dispose

dispose make refer to another dispose

Ehm sry - what do you mean?

you have multiple dispose declarations

So there is no way to link to the "correct" one?

I just tested and if I click on dispose it will get to such a page - is this good enough or should we trust that the unittest does the explanation?

this is not good enough

On 15 апр. 2016 г., at 18:08, Sebastian Wilzbach notifications@github.com wrote:

In source/mir/combinatorics/package.d #88 (comment):

@@ -251,6 +300,9 @@ Params:

Returns:
Forward range, which yields the permutations
+
+See_Also:

$(LREF Permutations), $(LREF dispose)
I just tested and if I click on dispose it will get to such a page - is this good enough or should we trust that the unittest does the explanation?

https://cloud.githubusercontent.com/assets/4370550/14567575/5b2cef92-033d-11e6-8fba-96ccd58c8996.png
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub https://github.com/DlangScience/mir/pull/88/files/21c97a5060d40c21e7e415ee78bab94c461e591f#r59898839

wilzbach · 2016-04-15T15:49:55Z

Hmm it seems that there were still a lot of stuff to do for the combinatorics module - here's a list of what i changed:

allocator order for makeCombinations,Repeat,CartesianPower
length for permutations (all other collections have this too)
unittests for length
unittest for invalid input
unittest for indexed
added const to @property attributes like bool and length
reworked the description for `IndexedRoR

9il · 2016-04-15T15:52:24Z

allocator order for makeCombinations,Repeat,CartesianPower

allocators should be first argumensts

wilzbach · 2016-04-15T15:53:32Z

allocators should be first argumensts

Yep those had the allocater as last argument

wilzbach · 2016-04-15T19:33:24Z

source/mir/combinatorics/package.d


-For example $(D"AB".combinations(2).array) returns $(D["AB"]).
+For example `"AB".combinations(2).array` returns `["AB"]`.



should I remove .array here?

remove this example from the docs. And add constraints, that templates are note accepts narrow strings.

wilzbach · 2016-04-15T19:34:37Z

@9il CI was failing due to an added tests which included the Mallocator - should be fixed now. Any other objections? :)

wilzbach · 2016-04-15T22:23:20Z

remove this example from the docs. And add constraints, that templates are note accepts narrow strings.

I created a module level unittest as an overview over the iterators.
Moreover I realized that indexed from std.range actually needs a random access range and thus corrected all template constraints. A narrow string can't be a random access string :)

wilzbach · 2016-04-15T23:17:52Z

(rebased to latest master - as we are moving quite fast tonight)

wilzbach · 2016-04-15T23:41:43Z

Failed to download http://code.dlang.org/packages/libevent/2.0.1+2.0.16.zip: 404 Not Found

why? :(

9il · 2016-04-15T23:44:23Z

why? :(

Just remove ddox. I don't like vibe.d and ddox ;/

We can use https://github.com/kiith-sa/harbored-mod, it is the best but needs small update. Probably we would need to add macros file from dlang

wilzbach · 2016-04-15T23:45:32Z

We can use https://github.com/kiith-sa/harbored-mod, it is the best but needs small update.

Okay will send a PR later ;-)

wilzbach · 2016-04-16T00:09:17Z

Okay will send a PR later ;-)

Hmm seems that we have to fix libdparse first ...
dlang-community/harbored-mod#62

wilzbach force-pushed the dscanner branch from 21bb020 to 3773367 Compare April 14, 2016 05:56

wilzbach mentioned this pull request Apr 14, 2016

CI linting guide dlang-community/D-Scanner#320

Open

wilzbach force-pushed the dscanner branch from 3773367 to a7788ae Compare April 14, 2016 05:59

wilzbach force-pushed the dscanner branch 8 times, most recently from 134a9a1 to 5599748 Compare April 14, 2016 14:04

wilzbach reviewed Apr 14, 2016
View reviewed changes

wilzbach mentioned this pull request Apr 14, 2016

fix style syntax for mir.ndslice #89

Closed

wilzbach reviewed Apr 15, 2016
View reviewed changes

wilzbach force-pushed the dscanner branch from 21c97a5 to 4e4ef14 Compare April 15, 2016 15:48

wilzbach force-pushed the dscanner branch from 4e4ef14 to a43bedc Compare April 15, 2016 15:57

wilzbach added the Style label Apr 15, 2016

wilzbach force-pushed the dscanner branch 5 times, most recently from a505745 to b73e74c Compare April 15, 2016 19:23

wilzbach reviewed Apr 15, 2016
View reviewed changes

wilzbach force-pushed the dscanner branch 2 times, most recently from 5ec45b5 to cbf3775 Compare April 15, 2016 22:22

wilzbach force-pushed the dscanner branch from cbf3775 to ca4cce0 Compare April 15, 2016 23:16

wilzbach added 2 commits April 16, 2016 02:18

add dscanner check

59b523b

removed a few style warnings

3ac2835

wilzbach force-pushed the dscanner branch from ca4cce0 to 3ac2835 Compare April 15, 2016 23:18

9il merged commit 2d5d46d into libmir:master Apr 15, 2016

wilzbach mentioned this pull request Apr 16, 2016

Please make a note in the README that libevent is required on OSX dlang/ddox#122

Closed


		For example $(D"AB".combinations(2).array) returns $(D["AB"]).
		For example `"AB".combinations(2).array` returns `["AB"]`.

add linting via dscanner #88

add linting via dscanner #88

Uh oh!

Conversation

wilzbach commented Apr 14, 2016

Uh oh!

codecov-io commented Apr 14, 2016

Current coverage is 98.17%

Uh oh!

9il commented Apr 14, 2016

Uh oh!

wilzbach commented Apr 14, 2016

Uh oh!

9il commented Apr 14, 2016

Uh oh!

wilzbach commented Apr 14, 2016

Naming style guide

Subtracting from .length

Zero-parameter '@Property' function

'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' should be const

has method 'opEquals', but not 'toHash'

Avoid use of the comma expression

Disabled flags

Uh oh!

Choose a reason for hiding this comment

Uh oh!

9il commented Apr 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

9il commented Apr 14, 2016

Uh oh!

9il commented Apr 14, 2016

Uh oh!

wilzbach commented Apr 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Apr 15, 2016

Uh oh!

9il commented Apr 15, 2016

Uh oh!

wilzbach commented Apr 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Apr 15, 2016

Uh oh!

wilzbach commented Apr 15, 2016

Uh oh!

wilzbach commented Apr 15, 2016

Uh oh!

wilzbach commented Apr 15, 2016

Uh oh!

9il commented Apr 15, 2016

Uh oh!

wilzbach commented Apr 15, 2016

Uh oh!

wilzbach commented Apr 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Current coverage is `98.17%`

Subtracting from `.length`