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 <jh...@apache.org> 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 <francischu...@apache.org> > 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 <zabe...@gmail.com> > > > 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 <mm...@apache.org> > wrote: > > >> > > >>> Thanks Stamatis! I haven't used SonarCloud before, but in general I > think > > >>> such tools can be quite useful. > > >>> > > >>> -- > > >>> Michael Mior > > >>> mm...@apache.org > > >>> > > >>> > > >>> On Sat, Dec 24, 2022 at 4:01 PM Stamatis Zampetakis < > zabe...@gmail.com> > > >>> 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 <libenc...@apache.org> > > >>> 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 <alessandro.solima...@gmail.com> > 于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 < > zabe...@gmail.com > > >>>> > > >>>>>> 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 > > >>>>> > > >>>> > > >>> > > >> > > > >
