Comments (25)
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
from manageiq-ui-service.
Wondering if we can just change the threshold, to make it less sensitive
https://docs.codeclimate.com/docs/duplication
from manageiq-ui-service.
@AllenBW @martinpovolny @AparnaKarve @h-kataria thoughts?
/cc @chessbyte @dclarizio @Fryguy
from manageiq-ui-service.
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.
from manageiq-ui-service.
@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.
from manageiq-ui-service.
@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?
from manageiq-ui-service.
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 🎱 💀 🍜
from manageiq-ui-service.
@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.
from manageiq-ui-service.
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...
from manageiq-ui-service.
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.
from manageiq-ui-service.
@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?
from manageiq-ui-service.
@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)
from manageiq-ui-service.
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..
from manageiq-ui-service.
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!
from manageiq-ui-service.
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 😄
from manageiq-ui-service.
@dtaylor113 what's the URL of those tests?
from manageiq-ui-service.
With the threshold set to 100, those violations do not show up anymore.
from manageiq-ui-service.
@AllenBW what are your thoughts about changing mass_threshold to 100?
@dtaylor113 I'd still like the URL
from manageiq-ui-service.
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.
from manageiq-ui-service.
from manageiq-ui-service.
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
from manageiq-ui-service.
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 :).
from manageiq-ui-service.
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..
from manageiq-ui-service.
@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.
from manageiq-ui-service.
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 :).
from manageiq-ui-service.
Related Issues (20)
- WS-2022-0284 (High) detected in moment-timezone-0.4.1.tgz, moment-timezone-0.5.33.tgz HOT 3
- WS-2022-0280 (High) detected in moment-timezone-0.4.1.tgz HOT 1
- WS-2022-0322 (High) detected in d3-color-1.4.1.tgz HOT 4
- CVE-2022-3517 (High) detected in minimatch-3.0.4.tgz - autoclosed HOT 1
- prepare the development environment to make code changes HOT 5
- CVE-2022-24999 (High) detected in qs-6.5.2.tgz - autoclosed HOT 1
- Unable to open a Native Console for a RHV VM in the Service UI
- CVE-2022-25881 (Medium) detected in http-cache-semantics-4.1.0.tgz - autoclosed HOT 1
- CVE-2023-28155 (Medium) detected in request-2.88.2.tgz - autoclosed HOT 1
- CVE-2023-26117 (Medium) detected in angular-1.8.3.tgz HOT 1
- CVE-2023-26118 (Medium) detected in angular-1.8.3.tgz HOT 1
- CVE-2023-26116 (Medium) detected in angular-1.8.3.tgz HOT 1
- CVE-2022-25883 (High) detected in semver-7.3.8.tgz - autoclosed HOT 1
- CVE-2020-23064 (Medium) detected in jquery-3.4.1.tgz, jquery-3.2.1.tgz - autoclosed HOT 1
- Opening the service ui results in downloading app.js over and over HOT 2
- CVE-2024-21490 (High) detected in angular-1.8.3.tgz
- CVE-2023-42282 (Critical) detected in ip-2.0.0.tgz - autoclosed HOT 1
- CVE-2024-28863 (Medium) detected in tar-6.2.0.tgz - autoclosed HOT 1
- Button role-based visibility rules do not work in UI Service HOT 2
- CVE-2023-44270 (Medium) detected in postcss-7.0.39.tgz
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from manageiq-ui-service.