I logged CALCITE-5474 [1] to disable the failures in Sonar checks allowing everything to appear as green.
Sonar annotations are still going to appear under the PR after CALCITE-5474; if the intention is to remove also these indications let me know and I will log another ticket. Best, Stamatis [1] https://issues.apache.org/jira/browse/CALCITE-5474 On Fri, Jan 13, 2023 at 2:53 AM Julian Hyde <[email protected]> wrote: > Now a build 'failed' with 7 'code smells'. > > Duplicating a string literal in a test was deemed as 'critical code > smell'. For heaven's sake. > > > https://sonarcloud.io/project/issues?resolved=false&severities=CRITICAL&types=CODE_SMELL&pullRequest=2942&id=apache_calcite > > Adding '@Deprecated' without also adding a javadoc comment that > contains '@deprecated' is a 'major code smell'. (I'm guessing that if > I add a javadoc comment that only contains '@deprecated' it will tell > me that empty javadoc is a code smell.) > > And "Do not forget to remove this deprecated code someday." is an > 'info code smell'. Yeah, right. Wait until the next major version. > > I'm trying to work here. Get this *****ing robot off my back. > > Julian > > On Wed, Jan 11, 2023 at 10:41 PM Alessandro Solimando > <[email protected]> wrote: > > > > Hi everyone, > > I agree with Julian that we should be open to see what value Sonar brings > > with the current setup, but for a true accounting we need many more data > > points, two examples are just not enough. > > > > In my experience I see reviewers asking contributors to fix real issues > > that Sonarlint plugin can highlight in IntelliJ even locally. > > > > So Sonar would save those reviewers time if the contributor would review > > and fix some of them autonomously. > > > > If we take Sonar as an opportunity to fix easily 80% (à la Pareto) of the > > trivial issues we generally see in contributions rather than considering > > each and every issue as blocking, we can have a positive net IMO. > > > > There are also fine tunings and exclusions to be added over time for > > accepted "issues" (like the test class under src), like Ruben was > proposing. > > > > I was the one who did the Sonar setup in Hive, I had mostly positive > > feedback by contributors who just took Sonar as an opportunity to fix > some > > bugs and improve code, the only difference is that we do not have any > > quality gate there, so the report is never marked as "failed", it's at > the > > sole discretion of the contributor+reviewer to take it into account or > not. > > > > I personally don't fix all possible warnings/code smells, but most of > them > > yes. Some are just fine as-is to me and they can even be considered false > > positives. > > > > Best regards, > > Alessandro > > > > > > On Wed 11 Jan 2023, 23:17 Julian Hyde, <[email protected]> wrote: > > > > > The instanceof case was complicated. The code that Kevin wrote was > good, > > > and appropriate, and when Sonar blocked it Stamatis was able to find an > > > alternative formulation, which existed because, by luck, the JSR had > > > deprecated an exception class but not its base class. I spent 30 > minutes > > > reviewing the PR and was about to merge it, and because of Sonar’s > bump in > > > the road that time was wasted. I doubt that there has been a single > other > > > occasion when someone wrote > > > “com.example.MyClass”.equals(x.getClass().getName()) instead of “x > > > instanceof MyClass”. So far that particular check has cost us ~1 hour > and > > > not saved us any time. > > > > > > I’m not saying that Sonar is net bad. I’m just saying let’s do a true > > > accounting. > > > > > > > > > > On Jan 11, 2023, at 2:42 AM, Ruben Q L <[email protected]> wrote: > > > > > > > > Hello, > > > > > > > > First of all, thanks Stamatis for implementing this, I think it is > > > > something good for the project. > > > > In the beginning things might be a bit complicated (as always) and we > > > might > > > > need some adjustments / clarifications, but I hope that in the long > run > > > > we'll see this as a useful feature. > > > > > > > > Regarding the two specific issues being discussed: > > > > - If I am not mistaken, the fact that SqlOperatorTest was moved out > of > > > > 'test' was a consequence of [1], see the corresponding PR [2] "... > it was > > > > necessary to move several classes from the 'core' module to > 'testkit'". I > > > > don't know how simple (or how complex) a potential refactoring to > move it > > > > out of there might be. Alternatively, it seems that this is rather an > > > > exceptional case, so perhaps it should qualify for an exception (e.g. > > > > everything under /testkit/* shall not be considered for coverage). > > > > - Regarding the instanceof, it seems that it was indeed a valid > warning, > > > > and it has recently been fixed via [3] (see discussion on its PR [4]) > > > > > > > > Best regards, > > > > Ruben > > > > > > > > [1] https://issues.apache.org/jira/browse/CALCITE-4885 > > > > [2] https://github.com/apache/calcite/pull/2685 > > > > [3] > > > > > > > > https://github.com/apache/calcite/commit/4bebdb07c2f45a95c9a4fdf81e9bcfbdd11a15de > > > > [4] https://github.com/apache/calcite/pull/2919 > > > > > > > > > > > > > > > > On Tue, Jan 10, 2023 at 3:18 PM Stamatis Zampetakis < > [email protected]> > > > > wrote: > > > > > > > >> Thanks for the feedback! > > > >> > > > >> I would like to stretch the fact that at this point it is at the > > > discretion > > > >> of the reviewer/committer to enforce or ignore the information > provided > > > by > > > >> Sonar. > > > >> > > > >> Sonar, as other similar systems, has limitations thus there are > always > > > >> going to be false positives. The rules/checks performed are fully > > > >> customisable so we can enable/disable them at will. > > > >> > > > >> The two issues highlighted by Julian seem like true positives to me. > > > >> * The "New code" that was introduced recently is not covered > > > sufficiently > > > >> by tests and that's a fact. Part of the problem seems to come from > the > > > >> recent modifications in SqlOperatorTest [1]. The class is located > under > > > >> src/main (and not under src/test) so it is considered as a > production > > > class > > > >> and coverage checks are applied. There are ways to exclude certain > paths > > > >> from coverage but I would argue that the class shouldn't be there > in the > > > >> first place; we should probably log a CALCITE ticket for moving out > of > > > >> there. > > > >> * The instance of warning is something that we probably don't > > > want/cannot > > > >> fix (for the reasons mentioned in the PR) but Sonar did well to > bring > > > this > > > >> up; it is problematic to do comparisons by relying on the class > name. > > > >> > > > >> I encourage others to share their thoughts/remarks as well so that > we > > > >> improve as much as possible the developer experience. > > > >> > > > >> Best, > > > >> Stamatis > > > >> > > > >> [1] > > > >> > > > >> > > > > https://github.com/apache/calcite/blob/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java > > > >> > > > >> On Tue, Jan 10, 2023 at 1:38 AM Julian Hyde <[email protected]> > wrote: > > > >> > > > >>> I see two false positives so far: > > > >>> * The message on be7135cf "58.1% Coverage on New Code (is less than > > > >> 80%)" > > > >>> * The bug on PR 2942 "Use an "instanceof" comparison instead" > > > >>> > > > >>> Has Sonarcube had any true positives yet? > > > >>> > > > >>> Vladimir used to hate when I was skeptical of changes to the build > > > >>> system. But I have no tolerance for a lint system that makes extra > > > >>> work. > > > >>> > > > >>> Julian > > > >>> > > > >>> > > > >>> > > > >>> > > > >>> On Mon, Jan 9, 2023 at 2:46 PM Francis Chuang < > > > [email protected]> > > > >>> wrote: > > > >>>> > > > >>>> Thanks for implementing this, Stamatis! Having code quality > metrics on > > > >>>> our repos is a huge win. > > > >>>> > > > >>>> Francis > > > >>>> > > > >>>> On 10/01/2023 4:22 am, Stamatis Zampetakis wrote: > > > >>>>> I just merged the CALCITE-5427 [1] enabling Sonarcloud analysis > for > > > >>> Calcite > > > >>>>> main branch and new PRs. > > > >>>>> > > > >>>>> I have left the default Sonar quality gates active so you may > start > > > >>> seeing > > > >>>>> Sonar reporting errors in various cases. > > > >>>>> > > > >>>>> If you encounter problems or you would like things to work > > > >> differently > > > >>>>> please create JIRA tickets and ping me if you need help. > > > >>>>> > > > >>>>> Once we are happy with how things work for Calcite we can also > port > > > >> the > > > >>>>> changes to Avatica. > > > >>>>> > > > >>>>> Note that all Calcite committers have administrative privileges > to > > > >> the > > > >>>>> Calcite project in Sonarcloud [2]. > > > >>>>> > > > >>>>> Best, > > > >>>>> Stamatis > > > >>>>> > > > >>>>> [1] > > > >>>>> > > > >>> > > > >> > > > > https://github.com/apache/calcite/commit/be7135cf1fd3d87e4036b2dd6e58d2f1251f8959 > > > >>>>> [2] https://sonarcloud.io/project/roles?id=apache_calcite > > > >>>>> > > > >>>>> On Wed, Jan 4, 2023 at 5:44 PM Stamatis Zampetakis < > > > >> [email protected]> > > > >>>>> wrote: > > > >>>>> > > > >>>>>> The integration is advancing well and I am hoping to merge the > PR in > > > >>> the > > > >>>>>> coming days. > > > >>>>>> To avoid unpleasant surprises, I am planning to create a new > remote > > > >>> branch > > > >>>>>> in the main calcite repo to test some things out. I will delete > it > > > >> as > > > >>> soon > > > >>>>>> as I am done with the tests. > > > >>>>>> > > > >>>>>> Best. > > > >>>>>> Stamatis > > > >>>>>> > > > >>>>>> On Wed, Dec 28, 2022 at 2:47 PM Michael Mior <[email protected]> > > > >>> wrote: > > > >>>>>> > > > >>>>>>> Thanks Stamatis! I haven't used SonarCloud before, but in > general I > > > >>> think > > > >>>>>>> such tools can be quite useful. > > > >>>>>>> > > > >>>>>>> -- > > > >>>>>>> Michael Mior > > > >>>>>>> [email protected] > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis < > > > >>> [email protected]> > > > >>>>>>> wrote: > > > >>>>>>> > > > >>>>>>>> Since there were no objections, I just logged INFRA-24038 [1] > and > > > >>> plan > > > >>>>>>> to > > > >>>>>>>> move this forward. > > > >>>>>>>> > > > >>>>>>>> Let me know if you have questions or concerns regarding the > > > >>> adoption of > > > >>>>>>>> SonarCloud. > > > >>>>>>>> > > > >>>>>>>> Best, > > > >>>>>>>> Stamatis > > > >>>>>>>> > > > >>>>>>>> [1] https://issues.apache.org/jira/browse/INFRA-24038 > > > >>>>>>>> > > > >>>>>>>> On Sat, Dec 10, 2022 at 11:32 AM Benchao Li < > [email protected] > > > >>> > > > >>>>>>> wrote: > > > >>>>>>>> > > > >>>>>>>>> Thanks Stamatis for bringing this up. > > > >>>>>>>>> > > > >>>>>>>>> I haven't used Sonar yet, but thanks for the demo[1] you > > > >> provided, > > > >>> it > > > >>>>>>>> looks > > > >>>>>>>>> really interesting. I think it's worth a try for Calcite. > > > >>>>>>>>> > > > >>>>>>>>> [1] https://github.com/zabetak/calcite/pull/9 > > > >>>>>>>>> > > > >>>>>>>>> Alessandro Solimando <[email protected]> > > > >>> 于2022年12月10日周六 > > > >>>>>>>>> 02:54写道： > > > >>>>>>>>> > > > >>>>>>>>>> Hi all, > > > >>>>>>>>>> thanks Stamatis for the proposal and the work, I am a huge > fan > > > >> of > > > >>>>>>> Sonar > > > >>>>>>>>> and > > > >>>>>>>>>> I really miss it on Calcite, so a big +1 from me on this. > > > >>>>>>>>>> > > > >>>>>>>>>> In Hive so far we have received good feedback, so I hope it > will > > > >>> be > > > >>>>>>>>>> welcomed here too. > > > >>>>>>>>>> > > > >>>>>>>>>> Best regards, > > > >>>>>>>>>> Alessandro > > > >>>>>>>>>> > > > >>>>>>>>>> On Fri, 9 Dec 2022 at 19:02, Stamatis Zampetakis < > > > >>> [email protected] > > > >>>>>>>> > > > >>>>>>>>>> wrote: > > > >>>>>>>>>> > > > >>>>>>>>>>> Hi all, > > > >>>>>>>>>>> > > > >>>>>>>>>>> I just logged CALCITE-5427 [1] for introducing code > quality & > > > >>>>>>>> coverage > > > >>>>>>>>>>> metrics in Calcite CI. > > > >>>>>>>>>>> > > > >>>>>>>>>>> I added some motivation and examples under the ticket so > please > > > >>>>>>> have > > > >>>>>>>> a > > > >>>>>>>>>> look > > > >>>>>>>>>>> and let me know what you think. > > > >>>>>>>>>>> > > > >>>>>>>>>>> If there are no objections, I will follow-up with INFRA to > set > > > >>>>>>> things > > > >>>>>>>>> up > > > >>>>>>>>>>> for the official Calcite repo. > > > >>>>>>>>>>> > > > >>>>>>>>>>> The integration with SonarCloud has been inspired by > HIVE-26196 > > > >>>>>>> [2] > > > >>>>>>>>> that > > > >>>>>>>>>>> Alessandro put in place for Hive and has been very helpful > so > > > >>> far. > > > >>>>>>>>>>> > > > >>>>>>>>>>> Best, > > > >>>>>>>>>>> Stamatis > > > >>>>>>>>>>> > > > >>>>>>>>>>> [1] https://issues.apache.org/jira/browse/CALCITE-5427 > > > >>>>>>>>>>> [2] https://issues.apache.org/jira/browse/HIVE-26196 > > > >>>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> -- > > > >>>>>>>>> > > > >>>>>>>>> Best, > > > >>>>>>>>> Benchao Li > > > >>>>>>>>> > > > >>>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>>> > > > >>> > > > >> > > > > > > >
