Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uniformed fixed issues presentation. #736

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

abramobagnara
Copy link

Previous fixed issues presentation is less featured than presentation of other issues with no added benefits.

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #736 (dd229ec) into master (13c4980) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #736   +/-   ##
=========================================
  Coverage     79.72%   79.73%           
  Complexity     1529     1529           
=========================================
  Files           243      243           
  Lines          5589     5590    +1     
  Branches        409      409           
=========================================
+ Hits           4456     4457    +1     
  Misses          970      970           
  Partials        163      163           
Impacted Files Coverage Δ Complexity Δ
...ins/plugins/analysis/core/model/DetailFactory.java 94.44% <100.00%> (+0.07%) 23.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13c4980...e937dd8. Read the comment docs.

@uhafner
Copy link
Member

uhafner commented Jan 9, 2021

Well, the added benefit is that it is a special and reduced view that renders the results in a simple way. I do not see a benefit to show the full capacity view for fixed warnings as well. This view will never be used to navigate to issues.

@abramobagnara
Copy link
Author

Well, the added benefit is that it is a special and reduced view that renders the results in a simple way. I do not see a benefit to show the full capacity view for fixed warnings as well. This view will never be used to navigate to issues.

I'm very surprised you say that: with the modified plugin we navigate the fixed issues all the time (to see what has been fixed).

@uhafner
Copy link
Member

uhafner commented Jan 9, 2021

Ok, seems you are using the plugin in a different way. So let's be more precise: what elements of the issues detail view are you using that is not part of the reduced view? How many fixed warnings do you typically have?

From jenkinsci/analysis-model#553 it looks like you are dealing with a lot of issues.

@abramobagnara
Copy link
Author

We have up to thousands of fixed issues in special cases.

The reduced view does not permit selection and does not show the issue details (compare e.g. https://ci.eclipse.org/skills/job/skills.build.nightly/111/analysis/new/ with https://ci.eclipse.org/skills/job/skills.build.nightly/111/analysis/fixed/ ).

If you think it is useful I can send to you privately the credentials to see the special uses we do of issue details to link the jenkins issue to our static analysis reports browser.

@uhafner
Copy link
Member

uhafner commented Jan 10, 2021

I see.

In this case it would make sense to completely remove the view (FixedWarningsDetail and jelly files).

@abramobagnara
Copy link
Author

I've removed it as you proposed.

@uhafner
Copy link
Member

uhafner commented Jan 12, 2021

Ok, thanks. I installed it on my machine and two things are now not ideal:

  • the link to the fixed source code is on the new file (it pointed to the file with the actual warning)
  • if there are no fixed warnings then the details look strange.

@abramobagnara
Copy link
Author

For the first point I agree it is suboptimal and if feasible it would be better to point to issue location in reference build.
For the second point I don't understand what you mean.

@uhafner
Copy link
Member

uhafner commented Jan 12, 2021

Bildschirmfoto 2021-01-13 um 00 48 47

@abramobagnara
Copy link
Author

This looks strange also for the New issues (only a litte less)...

What would you propose?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants