Hey Benchao,
I also see that issue when trying to log in via
https://sonarcloud.io/project/roles?id=apache_calcite. The solution is
to log in here: https://sonarcloud.io/login, go to the Apache
organization and then go to the calcite project.
Francis
On 16/01/2023 12:36 am, Benchao Li wrote:
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 <alessandro.solima...@gmail.com> 于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 <zabe...@gmail.com>
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 <jh...@apache.org> 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
<alessandro.solima...@gmail.com> 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, <jhyde.apa...@gmail.com>
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 <rube...@gmail.com>
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 <
zabe...@gmail.com>
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 <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
