When I open this url[1], it tells me that "You are not authorized to access this page". Is this expected? (I'm using my Github account)
[1] https://sonarcloud.io/project/roles?id=apache_calcite Alessandro Solimando <[email protected]> 于2023年1月13日周五 18:51写道： > Thank you Stamatis for working on this, I see no need to remove the Sonar > analysis now that the quality gate is off. > > In the meantime, since all committers have write privilege in Sonar, > whoever is interested can help fine-tuning it to fit our needs (for > instance disabling rules like the code smell for TODOs if unwanted), on a > voluntary basis, of course. > > Best regards, > Alessandro > > On Fri, 13 Jan 2023 at 11:23, Stamatis Zampetakis <[email protected]> > wrote: > > > 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 > > > > > >>>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > > -- Best, Benchao Li
