martin-helmich / typo3-typoscript-lint

Find coding errors in your TypoScript files.
MIT License
82 stars 19 forks source link

Claify commonPathPrefixThreshold use #57

Open mikestreety opened 6 years ago

mikestreety commented 6 years ago

Hey, thanks so much for this - it's going to be really helpful!

I was just wondering if the commonPathPrefixThreshold parameter under NestingConsistency could be clarified?

I have set it to 5 in my current project, so would expect it to flag items with 5 or more in common, however I get the following warnings:

Common path prefix "lib" with assignments to "lib.site" in line 167, "lib.navigation" in line 173, "lib.footer" in line 343, "lib.stdheader.10" in line 380, "lib.content" in line 525. Consider merging them into a nested assignment.

Common path prefix "plugin" with assignments to "plugin.tx_llcatalog_pi.models.news" in line 463, "plugin.tx_llcatalog_pi.models.users" in line 489, "plugin.tx_llcatalog_pi.models.video" in line 502, "plugin.tx_llcatalog_pi.models.offices" in line 512, "plugin.tx_llcatalog_pi.models.pardot" in line 517. Consider merging them into a nested assignment.

It wants me to merge items where there a 2 similarities.

We have it set to 5 as we have items such as the following as separate items

plugin.tx_llcatalog_pi.models.news {

}

plugin.tx_llcatalog_pi.models.video {

}

Maybe i'm misunderstanding the parameter. Mind clarifying please? Thank you!

oliverklee commented 6 years ago

@mikestreety I had similar warnings. Adapting my solutions for this to your case, the resulting TypoScript would look like this:

plugin.tx_llcatalog_pi.models {
  news {
  }
  video {
  }
}

Does this help?

mikestreety commented 6 years ago

Thanks @oliverklee, but I want these to pass (as it makes visually reading them easier).

I was more asking why these warning were coming up when the threshold is 5 - I would expect it to be 5 levels similar before flagging

martin-helmich commented 6 years ago

Ouch. I've had another look at the source code and it turns out that the naming for the commonPathPrefixThreshold option is wildly misleading.

What the option actually does is setting the minimum number of assignments that should exist with any common prefix before this rule starts firing. For example, with commonPathPrefixThreshold = 3, the following snippet (with less than three assignments to the lib. prefix) would be ok:

lib.foo = 1
lib.bar = 2

Whereas the following (with three (or more) assignments to the lib. prefix) will raise an error:

lib.foo = 1
lib.bar = 2
lib.baz = 3

I'll see if I can clear up the documentation behind this option a bit.

I can also understand the appeal of having an additional option that does exactly what you've assumed the first option will be doing. As soon as I can come up with a name that does not add to the confusion, I'll see about adding one.

mikestreety commented 6 years ago

This appears to be similar to the scss-lint "nesting depth" (although reversed).

Maybe something like nestingDepthSimilarity or something?

wolffc commented 5 years ago

Oh i was Under the Impression that commonPathPrefixThreshold is the segments of a path to be allowed Similar before it is counted.

we want to allow:

plugin.foo {
}
plugin.bar {
}
plugin.xta {
}
...

but we would want to trigger an waring if you define plugin.foo twice

this should like raise a warning

plugin.bar {
}
plugin.bar {
}