You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by oignatenko <oi...@gridgain.com> on 2018/11/28 16:40:22 UTC
Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Hello,
When discussing issues involved in IGNITE-10399 we have got an idea to
change color highlighting of some code style inspections to make it easier
to see which can lead to problems at Teamcity.
- Screen shot of how it is supposed to work:
https://issues.apache.org/jira/secure/attachment/12949868/IDEA.inspections.TC-bot.png
^^^ Violations of inspections used by TC are shown as red in IDEA Error
Stripe while the rest remains yellow, looks easy to tell one from another.
(It's probably not very important but for the sake of completeness a thing I
noticed when testing this is that having red inspections didn't block
compilation and execution of the code.)
- JIRA ticket to change inspections:
https://issues.apache.org/jira/browse/IGNITE-10450
- Prior discussions related to inspections checks at Teamcity:
http://apache-ignite-developers.2346864.n4.nabble.com/Code-inspection-tp27709.html
Your feedback is welcome.
TIA,
regards, Oleg
PS. Special thanks to Maxim Muzafarov for help in brainstorming this matter
and in drafting JIRA ticket.
--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by Павлухин Иван <vo...@gmail.com>.
Hi Oleg,
Thank you for the story. Now I see the concerns better.
чт, 29 нояб. 2018 г. в 18:45, oignatenko <oi...@gridgain.com>:
>
> Hi Ivan,
>
>
> Павлухин Иван wrote
> > P.S. Did not really get what was the main problem related to
> > IGNITE-10399 [1]. I suppose it should go: simple problem -- fast fix.
>
> Well, speaking of IGNITE-10399 it turned out that problem is indeed simple
> and fix was fast, just as you wrote.
>
> What looked wrong is that discovery of the problem took quite long and it
> turned out to be too much effort for such a simple problem and fix.
>
> To start with, it is essentially impossible to find out when coding. You can
> see why is that at screen shot reproducing how missing import looked like in
> the file that caused this issue (attached to IGNITE-10450):
> https://issues.apache.org/jira/secure/attachment/12950020/IDEA.inspections.TC-bot.obscured.png
>
> Please notice how mark denoting troublesome inspection violation is buried
> among non-critical ones which makes it very easy to miss when coding.
>
> Another problem is, the only reliable way to tell if there is an issue is to
> run Teamcity checks. Which means one needs to push and wait for quite a
> while to just find out if there is a problem or not (note by the way how
> this way breaks if one is working offline or if Teamcity server is down /
> busy for some reason).
>
> And this is not yet the end - even when you learn that some inspection
> failed somewhere there is no easy way to just get back to your code and find
> it - because as you can see from above screen shot the problem is obscured
> too much to notice. One needs to get to Teamcity report and read details of
> the failure to find out where to look for it.
>
> Above sounds a bit too much for finding and fixing a simple missing import
> isn't it?
>
> Do we really need to push, launch TC checks, wait for completion and read
> detailed report to simply find out an issue that was already reported and
> highlighted in IDE before all that, only hard to notice.
>
> ---
>
> When I realized that I thought maybe I would prefer if IDE could highlight
> such inspections more prominently to let me find it just when I introduce
> them during coding, without cumbersome messing with Teamcity checks and
> reports and I went to Maxim and he helped me find how this could be done.
>
> And after checking how it would work I created IGNITE-10450 for us to try
> this way - because it looked so much more convenient compared to what I
> observed in IGNITE-10399.
>
> regards Oleg
>
>
>
> --
> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
--
Best regards,
Ivan Pavlukhin
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by oignatenko <oi...@gridgain.com>.
Hi Ivan,
Павлухин Иван wrote
> P.S. Did not really get what was the main problem related to
> IGNITE-10399 [1]. I suppose it should go: simple problem -- fast fix.
Well, speaking of IGNITE-10399 it turned out that problem is indeed simple
and fix was fast, just as you wrote.
What looked wrong is that discovery of the problem took quite long and it
turned out to be too much effort for such a simple problem and fix.
To start with, it is essentially impossible to find out when coding. You can
see why is that at screen shot reproducing how missing import looked like in
the file that caused this issue (attached to IGNITE-10450):
https://issues.apache.org/jira/secure/attachment/12950020/IDEA.inspections.TC-bot.obscured.png
Please notice how mark denoting troublesome inspection violation is buried
among non-critical ones which makes it very easy to miss when coding.
Another problem is, the only reliable way to tell if there is an issue is to
run Teamcity checks. Which means one needs to push and wait for quite a
while to just find out if there is a problem or not (note by the way how
this way breaks if one is working offline or if Teamcity server is down /
busy for some reason).
And this is not yet the end - even when you learn that some inspection
failed somewhere there is no easy way to just get back to your code and find
it - because as you can see from above screen shot the problem is obscured
too much to notice. One needs to get to Teamcity report and read details of
the failure to find out where to look for it.
Above sounds a bit too much for finding and fixing a simple missing import
isn't it?
Do we really need to push, launch TC checks, wait for completion and read
detailed report to simply find out an issue that was already reported and
highlighted in IDE before all that, only hard to notice.
---
When I realized that I thought maybe I would prefer if IDE could highlight
such inspections more prominently to let me find it just when I introduce
them during coding, without cumbersome messing with Teamcity checks and
reports and I went to Maxim and he helped me find how this could be done.
And after checking how it would work I created IGNITE-10450 for us to try
this way - because it looked so much more convenient compared to what I
observed in IGNITE-10399.
regards Oleg
--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by Павлухин Иван <vo...@gmail.com>.
Hi Oleg,
First of all I am not against giving it a try. Let's try it.
P.S. Did not really get what was the main problem related to
IGNITE-10399 [1]. I suppose it should go: simple problem -- fast fix.
[1] https://issues.apache.org/jira/browse/IGNITE-10399
чт, 29 нояб. 2018 г. в 16:04, oignatenko <oi...@gridgain.com>:
>
> Hi Ivan,
>
> You are right that these things will distract people and from this
> perspective it is very well justified that vast majority of style deviations
> (currently, all of them) are marked yellow. These are non-critical and if
> developer ignores them nothing immediately bad happens.
>
> (For the sake of completeness one can argue that style deviations contribute
> to technical debt but it's tangential to current discussion and here we can
> probably consider them just harmless for simplicity.)
>
> The problem with five of these inspections that are proposed to change is,
> these became different after IGNITE-9983 and above reasoning doesn't work
> for these anymore. These five inspections are now checked at Teamcity and
> when there are new deviations it reports problems.
>
> Essentially this means that developer introducing new violations in these
> inspections is going to be distracted anyway - if they ignore at the coding
> phase they still will be chased by the warnings from Teamcity. You can check
> details of IGNITE-10399 because it shows a good example of how it goes.
>
> So what we're discussing here is essentially not about whether to distract
> developer or not (because they will be distracted anyway) but when it is
> more convenient to distract - at coding time or after Teamcity check.
> Granted, delaying this to TC checks felt okay to me before we tried it but
> observing how it really goes (in mentioned above IGNITE-10399) made me
> curious if maybe we could try another option by raising this issue at coding
> time instead.
>
> This is the whole point of this change, to let us try how it would go if we
> warn developers about inspections impacting Teamcity in the time of coding.
> As I wrote in beginning of this thread I briefly tried it myself and it
> looked quite promising.
>
> To avoid misunderstanding, I would like to make it clear that at this point
> it is not supposed to be a one way change because my testing was too brief
> to say for sure if this is the way to go. Current plan is that we give it a
> try for a while and later - depending on how folks feel - decide whether to
> keep these inspections red or revert them back to yellow.
>
> Does that make sense Ivan?
>
> regards, Oleg
>
>
>
>
> --
> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
--
Best regards,
Ivan Pavlukhin
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by oignatenko <oi...@gridgain.com>.
Hi Ivan,
You are right that these things will distract people and from this
perspective it is very well justified that vast majority of style deviations
(currently, all of them) are marked yellow. These are non-critical and if
developer ignores them nothing immediately bad happens.
(For the sake of completeness one can argue that style deviations contribute
to technical debt but it's tangential to current discussion and here we can
probably consider them just harmless for simplicity.)
The problem with five of these inspections that are proposed to change is,
these became different after IGNITE-9983 and above reasoning doesn't work
for these anymore. These five inspections are now checked at Teamcity and
when there are new deviations it reports problems.
Essentially this means that developer introducing new violations in these
inspections is going to be distracted anyway - if they ignore at the coding
phase they still will be chased by the warnings from Teamcity. You can check
details of IGNITE-10399 because it shows a good example of how it goes.
So what we're discussing here is essentially not about whether to distract
developer or not (because they will be distracted anyway) but when it is
more convenient to distract - at coding time or after Teamcity check.
Granted, delaying this to TC checks felt okay to me before we tried it but
observing how it really goes (in mentioned above IGNITE-10399) made me
curious if maybe we could try another option by raising this issue at coding
time instead.
This is the whole point of this change, to let us try how it would go if we
warn developers about inspections impacting Teamcity in the time of coding.
As I wrote in beginning of this thread I briefly tried it myself and it
looked quite promising.
To avoid misunderstanding, I would like to make it clear that at this point
it is not supposed to be a one way change because my testing was too brief
to say for sure if this is the way to go. Current plan is that we give it a
try for a while and later - depending on how folks feel - decide whether to
keep these inspections red or revert them back to yellow.
Does that make sense Ivan?
regards, Oleg
--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by Павлухин Иван <vo...@gmail.com>.
Hi Oleg,
I am not ultimately sure that it will not do any harm. Red error
highlights are designed to attract attention very well. I can imagine
that I will be distracted by every unused import. While usually I rely
on "Optimize imports" enabled in the commit dialog in IDEA.
But I think that we can try it.
ср, 28 нояб. 2018 г. в 21:12, Dmitriy Pavlov <dp...@apache.org>:
>
> Sure, I agree to let the discussion run its course and give it a couple of
> days so people have a chance to think and chime in.
>
> I just wanted to show I'm ++1 here and probably we can employ
> Commit-Than-Review later
>
>
> ср, 28 нояб. 2018 г. в 20:40, oignatenko <oi...@gridgain.com>:
>
> > Hi Dmitry,
> >
> > When we had preliminary discussion of this with Maxim we both were inclined
> > to post it here and let it hang for a while to let dev list folks discuss
> > this idea in more details for just in case if we missed some usability
> > implications.
> >
> > Though now that you mentioned it I figured that proposed change is low risk
> > and easy to rollback, meaning we can do it the other way round: just merge
> > it now and keep in mind an option to revert if further discussion here
> > shows
> > that this way is wrong for some reason.
> >
> > If you prefer that we pick this way, changing priorities for TC inspections
> > could even be done as a part of another ticket involving this config file,
> > IGNITE-10422 - you can probably discuss with Maxim if he would be
> > comfortable with that (last time I checked he planned to do implementation
> > there).
> >
> > regards, Oleg
> >
> >
> >
> > --
> > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
> >
--
Best regards,
Ivan Pavlukhin
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by oignatenko <oi...@gridgain.com>.
Following up prior discussion in this thread, suggested changes were merged
to master as a part of IGNITE-10422 and are now included in project default
inspections profile.
Thanks to Maxim and Dmitriy for making it happen.
As for the ticket IGNITE-10450, I plan to keep it open for a little while
for just the case if we quickly decide to do some adjustments. Then I plan
to close it (assuming that if we will want to change something later this
could be done per new tickets).
regards, Oleg
--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by Dmitriy Pavlov <dp...@apache.org>.
Sure, I agree to let the discussion run its course and give it a couple of
days so people have a chance to think and chime in.
I just wanted to show I'm ++1 here and probably we can employ
Commit-Than-Review later
ср, 28 нояб. 2018 г. в 20:40, oignatenko <oi...@gridgain.com>:
> Hi Dmitry,
>
> When we had preliminary discussion of this with Maxim we both were inclined
> to post it here and let it hang for a while to let dev list folks discuss
> this idea in more details for just in case if we missed some usability
> implications.
>
> Though now that you mentioned it I figured that proposed change is low risk
> and easy to rollback, meaning we can do it the other way round: just merge
> it now and keep in mind an option to revert if further discussion here
> shows
> that this way is wrong for some reason.
>
> If you prefer that we pick this way, changing priorities for TC inspections
> could even be done as a part of another ticket involving this config file,
> IGNITE-10422 - you can probably discuss with Maxim if he would be
> comfortable with that (last time I checked he planned to do implementation
> there).
>
> regards, Oleg
>
>
>
> --
> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
>
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by oignatenko <oi...@gridgain.com>.
Hi Dmitry,
When we had preliminary discussion of this with Maxim we both were inclined
to post it here and let it hang for a while to let dev list folks discuss
this idea in more details for just in case if we missed some usability
implications.
Though now that you mentioned it I figured that proposed change is low risk
and easy to rollback, meaning we can do it the other way round: just merge
it now and keep in mind an option to revert if further discussion here shows
that this way is wrong for some reason.
If you prefer that we pick this way, changing priorities for TC inspections
could even be done as a part of another ticket involving this config file,
IGNITE-10422 - you can probably discuss with Maxim if he would be
comfortable with that (last time I checked he planned to do implementation
there).
regards, Oleg
--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
Re: Change code style inspections to use red mark for those used at
Teamcity build checks (IGNITE-10450)
Posted by Dmitriy Pavlov <dp...@apache.org>.
Hi Oleg,
Let's merge the change if we already have some, WDYT?
Sincerely,
Dmitriy Pavlov
ср, 28 нояб. 2018 г. в 19:40, oignatenko <oi...@gridgain.com>:
> Hello,
>
> When discussing issues involved in IGNITE-10399 we have got an idea to
> change color highlighting of some code style inspections to make it easier
> to see which can lead to problems at Teamcity.
>
> - Screen shot of how it is supposed to work:
>
>
> https://issues.apache.org/jira/secure/attachment/12949868/IDEA.inspections.TC-bot.png
> ^^^ Violations of inspections used by TC are shown as red in IDEA Error
> Stripe while the rest remains yellow, looks easy to tell one from another.
> (It's probably not very important but for the sake of completeness a thing
> I
> noticed when testing this is that having red inspections didn't block
> compilation and execution of the code.)
>
> - JIRA ticket to change inspections:
> https://issues.apache.org/jira/browse/IGNITE-10450
>
> - Prior discussions related to inspections checks at Teamcity:
>
>
> http://apache-ignite-developers.2346864.n4.nabble.com/Code-inspection-tp27709.html
>
> Your feedback is welcome.
>
> TIA,
> regards, Oleg
>
> PS. Special thanks to Maxim Muzafarov for help in brainstorming this matter
> and in drafting JIRA ticket.
>
>
>
> --
> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
>