Enumerating rules - Githubissues

AndreaCrotti commented 7 years ago

I just wanted to see myself where was this rule defined and if I could simply fix it maybe.

Differently from most other linting tools however the kibit report doesn't contain any code on each error, making it more complicated to know what it's referring to exactly.

I think it would be great to give a unique identifier to each rule and output that as well in the report. This would also allow other cool things like filtering on certain rules, excluding checks and so on and so forth..

danielcompton commented 7 years ago

Yep, I've thought this would be a handy feature to have for lots of different purposes. Happy to see a PR for this.

AndreaCrotti commented 7 years ago

Yes just a few questions/ideas: Would it be fine just to categorize as they are already now? For example misc-01, collections-01 etc.

Are all the equivalences considered equal? Or some "mistakes" can be considered worse than others (as for linting, where you have errors/warnings).

So for the implementation I think one way could be to define each rule like this instead:

  :misc-01
  {:rule [(apply str (interpose ?x ?y)) (clojure.string/join ?x ?y)]
   :verbose-name "Interspose"}

That would give an identifier and another potential string with a more verbose explanation. Would that be fine @danielcompton ?

danielcompton commented 7 years ago

Are all the equivalences considered equal?

I haven't looked for a while, but I'm 99% sure that all of the rules are transformations on code which should be transparent to any callers, i.e. Kibit suggests style changes, but doesn't catch broken code.

One possibility for syntax:

(defrules rules
          ;; clojure.string
          {:rule [(apply str (interpose ?x ?y)) (clojure.string/join ?x ?y)]
           :id   :interpose->string-join
           :name "string/join instead of interpose"
           :explanation "Prefer clojure.string/join instead of interpose for string building."}
          {:rule [(apply str (reverse ?x)) (clojure.string/reverse ?x)]
           :id :reverse->string-reverse
           :name "string/reverse instead of reverse"
           :explanation "Prefer clojure.string/reverse instead of clojure.core/reverse"}
          [(apply str ?x) (clojure.string/join ?x)] )

The nice thing about this layout is that the rules can be converted and upgraded over time.

Thoughts about the metadata:

:id - id's need to be stable and long-lasting. I think it could be good to take advantage of namespaced keywords, so that the rule id's would get prefixed with the namespaced keywords. Tying them to the namespace they are defined in is perhaps too rigid though? I can see the benefits of using numeric ID's, and how they would make it easy to define id's, and rearrange things.
:name - Names are always hard. I'm not sure if there is a strong benefit for giving a name and an explanation, as the name is just a shortening of the explanation, and the code transformations are usually pretty self explanatory.
:explanation - While the explanation is somewhat useful, I'm not sure how much benefit it gives over and above just seeing the code transformation. However I know tools like @codeclimate use Kibit, so this might be good for them? @pbrisbin what would be useful for your team here?

Perhaps hold off on implementation for a little bit longer so we can see what other use cases come up that we need to take into account?

pbrisbin commented 7 years ago

Thanks for mentioning me @danielcompton.

What we might use a field like :explanation for would be the content.body issue property, which ultimately ends up rendered by the Book icon on the site. It's meant as guiding content that describes the motivation behind, and ideally suggests ways to fix, the Issue.

It looks like we presently build this content from the before/after snippets: https://github.com/codeclimate/codeclimate-kibit/blob/master/src/codeclimate/kibit.clj#L19

I'm not a heavy Kibit user, but the current approach seems reasonable to me. So I wouldn't add an :explanation just for us, though we would certainly consider using one if/when it was available.

AndreaCrotti commented 7 years ago

I think there might be quite a bit of repetition between id/name/explanation potentially, specially for the very simple sustitutions.

And the other thing is that normally most linting tools define some alphanumeric (shorter) identifier for each warning, which I think generally makes it easier to skip/filter/etc. So if I can make a suggestion I would rather do

:id coll-01
:name interpose->string-join
:rule [(apply str (interpose ?x ?y)) (clojure.string/join ?x ?y)]

And explanation can also be there sure but could probably be optional..

It's easy to then have a table lookup in the docs to check what each id mean and use them for all the filtering you want.. I'm afraid otherwise just using names could potentially generate confusing/very long names/

What do you think @danielcompton @pbrisbin ?

AndreaCrotti commented 7 years ago

And by the way how would you think about doing a change like this @danielcompton ? The actual annotation part will be potentially quite long and so at least I would say we need to:

make the code support this new structure (with just :rule in the beginning and leaving everything optional_
expand all the rules annotations
once it's all done potentially make actual use of the :id and :name fields as well.

Do the rules need to be annotated all together?

AndreaCrotti commented 7 years ago

Any news on this? I could also start to put something together but just wanted to know if my plan mentioned above was fine before doing it..

danielcompton commented 7 years ago

Hey, that sounds like a pretty good starting point. It's a little bit hard to imagine without seeing the output/configuration, but I think it's a good place to start from.

Do the rules need to be annotated all together?

I don't think so, possibly defrules syntax similar to what I showed in https://github.com/jonase/kibit/issues/175#issuecomment-284514902 could be good?

AndreaCrotti commented 7 years ago

Mm well @danielcompton as I mentioning https://github.com/jonase/kibit/issues/175#issuecomment-284575205

That example you gave is a bit repetitive, and the :id is not alphanumeric, which for me generally makes it much easier to make the ids unique and enumerate all the rules..

danielcompton commented 7 years ago

Oh sure, I agree with your plans, I was just meaning this syntax:

(defrules rules
          ;; clojure.string
          <map or vector repeated here>
          [(apply str ?x) (clojure.string/join ?x)] )

would allow you to migrate things over slowly.

clj-commons / kibit

Enumerating rules #175