CodeClimate issues that should not block PR merge about manageiq-ui-service

[chriskacerguis]

Ok, so given the discussion on this, I can get behind moving the mass_threshold to 100.

We didn't really talk about the larger issue of "CodeClimate issues that should not block PR merge", we seemed to focus on the individual use case. That said, I do think that CodeClimate gives us a lot of advantages, and so if CC "fails" then we should not merge.

/cc @AllenBW @martinpovolny @AparnaKarve @h-kataria @chessbyte @dclarizio @Fryguy

[chriskacerguis]

Wondering if we can just change the threshold, to make it less sensitive

https://docs.codeclimate.com/docs/duplication

[chriskacerguis]

@AllenBW @martinpovolny @AparnaKarve @h-kataria thoughts?

/cc @chessbyte @dclarizio @Fryguy

[dtaylor113]

Hi, I raised the Javascript mass_threshold to 100, this reduced the amount of issues to a manageable size in my PR. And more importantly, seems to identify only 'real' issues now.

[h-kataria]

@chriskacerguis i think we should open a new GH issue to address these kinds of valid warnings/issues to be addressed separately outside of the existing PR and not hold off on merging of the PR.

[chriskacerguis]

@h-kataria at what point do you foresee they get addressed (timeline wise)? It seems that with how things go, we quickly accumulate debt.

@dtaylor113 why 100? Is there a reason for that number, or is it just an arbitrary high number. If the CC scanned PR's, the "false positives" seem to be between 40 - 50 (default threshold is 40). So, I wonder if setting the threshold to 60 would be sufficient. Thoughts?

[AllenBW]

oooo i like that idea @h-kataria, and would add, with reference to timeline, next issue to be worked by the individual would be the resolution of the code climate deets

this might take some self control though 🎱 💀 🍜

[chriskacerguis]

@AllenBW yeah I do think that @h-kataria idea has a lot of merit, but it's the coming back to it part that I worry about. Which may have nothing to do with the desire of dev to come back it to, they may just not have time.

[AllenBW]

so may a requirement that before pr is accepted that ya reference the issue being worked that address climate issues?

but that'll only be as helpful as it is observed...

[AparnaKarve]

First and foremost, is this particular code duplication issue fixable?
And even if it is, would we be compromising on the readability of the code?

I think we should evaluate things on a case-by-case basis.
For this specific duplication case, I think we should ignore CodeClimate if there is no fix in the foreseeable future.

[chriskacerguis]

@AparnaKarve interesting, who do you propose makes the call if there is "no fix in the foreseeable future"? Or maybe what is the criteria for that?

[dtaylor113]

@chriskacerguis , looking at the violations I was getting, 100 seemed the cut off. mass-74 was the directive duplication, likewise mass-84 was the pf directive sort config. Putting it to 100, cleared a lot of the questionable ones, but did leave the larger code duplication -all the list view issues, which I thought was appropriate (note, I'm adding a list service to handle the repeated filtering)

[himdel]

Easy.. if the PR is introducing the issue, which wasn't there previously, block the PR until it fixes the issue. Else .. if a PR solving the issue already exist, and doesn't seem to be blocked, wait for that PR. Else .. if neither a PR nor an issue exists, create the issue, either way, link it to the PR. That way, we'll ensure that the ignored warning won't get lost or forgotten.

The second step is obviously that we need somebody to be working on those issues, but well, we need to be working on all the issues, so.. that's reducing the problem to one already solved :). And yes, this breaks if nobody does work on those, but, well, that's our problem and not really a reason to block unrelated PRs :).

Honestly I suspect it won't be a problem in the long run, or at least, if it is, we'll notice by having an enormous number of issues, and can react to it by focusing on these a bit more..

[dtaylor113]

When we set the duplication mass_threshold to less than 100, we start getting these:

This is how we define modal dialogs in SUI!

This last one I don't even know how we would fix, as it is the standard angular directive code pattern!

[dtaylor113]

IMO mass_threshold of 100 seemed to be the sweet spot. At this setting Code Climate showed me several similar code blocks, which I could do something about! I had no problem accepting and fixing those violations 😄

[chriskacerguis]

@dtaylor113 what's the URL of those tests?

[dtaylor113]

With the threshold set to 100, those violations do not show up anymore.

[chriskacerguis]

@AllenBW what are your thoughts about changing mass_threshold to 100?

@dtaylor113 I'd still like the URL

[dtaylor113]

Well, I was for changing it to 100 'cause it trimmed down the violations to a manageable amount, but I was under the impression that all code climate violations had to be resolved in order for a PR to be merged, but I see that you merged Jeff's PR which still had CC violations.
If CC doesn't block PRs, then maybe 100 is too high, but (a lot of 'buts' here :-) as I wrote in #204, I was getting CC violations at mass = 93 for the way we setup dialogs -I'm not sure how to resolve some of those.

[dtaylor113]

Oh, url is https://codeclimate.com/github/dtaylor113/manageiq-ui-self_service/compare/blueprint-list-unit-test#ratings

[AllenBW]

Increasing mass threshold tells the codeclimate duplication engine "code blocks of this mass(size) that are identical or similar are ok". Codeclimate defines similar as

When 2 or more blocks of code contain the same structure, but have different contents (such as variable names or literal values).

Large patterns already exist in the sui code base, an example would be our state controllers. FYSA default is 40. Given our app size, design patterns employed and that the default is only a suggestion, I would say increasing the very neat threshold to 100 is fine.

https://docs.codeclimate.com/docs/duplication

[himdel]

That said, I do think that CodeClimate gives us a lot of advantages, and so if CC "fails" then we should not merge.

Actually, can we have a discussion on that too?

I do agree that we should respect code climate and prefer not to merge when red. That said, let's think about it - if we have a CC failure that's clearly wrong, and a PR that's clearly doing the right thing - what should we do? Block a perfectly sensible PR because CC is magic, or merge it and then try to address the CC failure?

To me, @dtaylor113 's PR is a clear example of this, it should not have been blocked by any of this.

What I'm afraid of is the culture of naah, it's red, I can't merge because I don't understand the problem. Tools fail. So, I'd propose something like - merge, but create an issue about it, so that we can at least track the problems. Just, don't block a sane PR for two weeks until we figure out how to fix codeclimate :).

[himdel]

Just realized one more thing.. CC is essentially doing the same thing here that Rubocop is doing in the ops UI. In the ops UI, we simply ignore rubocop warnings that are not critical or explicitly wrong. So in effect, not doing the same thing here would constitute a policy change..

[chriskacerguis]

@himdel I mostly agree with you there is a culture of "it's red, I can't merge and I don't want to take the time to understand the problem". However, I think that, in the case of @dtaylor113 this "system" worked. He raised a flag, we talked, and I felt that it was dealt with in a reasonable amount of time.

The big thing for me, is that I feel creating a "I'll come back to it later" is problematic; and leads to technical debt. This repo had (when I started) about 31% coverage, and I wonder if that "I'll come back to you" culture has caused some of that. Just a thought.

And yes, it's similar to Rubocop, but also does a few other things.

[himdel]

Right, I definitely agree that "i'll come back to it later" often means never, or practically never :(. But then again, not always, so I still have hope :)

And really, I'm not trying to push us towards ignoring CC, that's the last thing I'd want.. I guess all I really want is something like "never merge with red CC, unless it makes sense" :D but then we're back to people and what who interprets how..

I really don't know, leaving it up to you, until it bites me :).