Page MenuHomeSoftware Heritage

scanner: dashboard file visualization per directory path
ClosedPublic

Authored by DanSeraf on Tue, Jun 16, 6:31 PM.

Details

Summary

Dashboard file browser (Related T2364)

Display the files relative to the directory path selected in the sunburst chart.

Output example:

Diff Detail

Repository
rDTSCN Code scanner
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

DanSeraf created this revision.Tue, Jun 16, 6:31 PM

Build is green

Patch application report for D3293 (id=11673)

Rebasing onto bf9e586436...

Current branch diff-target is up to date.
Changes applied before test
commit 6aa1c86f3cb64a79e230fa312b7b8c71d22fb38c
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Tue Jun 16 18:24:56 2020 +0200

    dashboard: file visualization per directory path

See https://jenkins.softwareheritage.org/job/DTSCN/job/tests-on-diff/30/ for more details.

DanSeraf edited the summary of this revision. (Show Details)Tue, Jun 16, 6:45 PM

Two quick comments, I'll do an actual review on thursday:

  • I don't think adding an external dependency (the .css) is a good idea (it might change or disappear). But this file doesn't seem to have a license so we probably can't bundle it either (ping @zack )
  • It's missing tests. I understand it's not easy to do for a GUI, but could you see what you can do about it?
zack added a comment.Wed, Jun 17, 9:12 AM
  • I don't think adding an external dependency (the .css) is a good idea (it might change or disappear). But this file doesn't seem to have a license so we probably can't bundle it either (ping @zack )

Yeah, agreed.

To use that CSS we should either incorporate it (incurring in the copyright and/or license problem mentioned by @vlorentz), or hotlink to it (incurring in tracking/404 problems). Neither of which is great.

We should either find another CSS which is FOSS-licensed, or roll our own—the former option being preferable.

DanSeraf updated this revision to Diff 11758.Fri, Jun 19, 4:41 PM
  • css as static asset

Build has FAILED

Patch application report for D3293 (id=11758)

Rebasing onto bf9e586436...

Current branch diff-target is up to date.
Changes applied before test
commit 536bb0fca288651365c594165a1237f736b0a5f2
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Tue Jun 16 18:24:56 2020 +0200

    dashboard: file visualization per directory path

Link to build: https://jenkins.softwareheritage.org/job/DTSCN/job/tests-on-diff/31/
See console output for more information: https://jenkins.softwareheritage.org/job/DTSCN/job/tests-on-diff/31/console

DanSeraf updated this revision to Diff 11759.Fri, Jun 19, 4:51 PM
  • init.py in dashboard directory

Build is green

Patch application report for D3293 (id=11759)

Rebasing onto bf9e586436...

Current branch diff-target is up to date.
Changes applied before test
commit 8c28d2614893b44e6f7bca81ab71a7274bd90395
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Tue Jun 16 18:24:56 2020 +0200

    dashboard: file visualization per directory path

See https://jenkins.softwareheritage.org/job/DTSCN/job/tests-on-diff/32/ for more details.

  • It's missing tests. I understand it's not easy to do for a GUI, but could you see what you can do about it?

Regarding the tests, i don't know if testing the length of the returning table could be useful.
Unfortunately the expected values can't be tested.

Unfortunately the expected values can't be tested.

Why not?

I must be missing something; how is the new .css file included from the html?

Unfortunately the expected values can't be tested.

Why not?

Because the dash_html_components checks for object identity only. I wrote a comment inside the test.

I must be missing something; how is the new .css file included from the html?

Since the dash_boostrap_components uses Bootstrap as CSS, it's better to use it also to manage the columns width.

DanSeraf updated this revision to Diff 11772.Sat, Jun 20, 4:37 PM
  • workaround to check the table body
  • bootstrap css update

Build is green

Patch application report for D3293 (id=11772)

Rebasing onto bf9e586436...

Current branch diff-target is up to date.
Changes applied before test
commit a2efb5490b09a8433773c8ad43b8021077dcb19a
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Tue Jun 16 18:24:56 2020 +0200

    dashboard: file visualization per directory path

See https://jenkins.softwareheritage.org/job/DTSCN/job/tests-on-diff/33/ for more details.

@zack How do you feel about the minified bootstrap code?

zack added a comment.Mon, Jun 22, 10:31 AM

@zack How do you feel about the minified bootstrap code?

Ideally we should have both the minified and non-minified version in Git.

(Even better would be generating the minified from the non-minified at deploy time, but that will inflate the deployment dependencies by a lot, so I don't think it's worth it for something that is not primarily a web app.)

DanSeraf updated this revision to Diff 11798.Mon, Jun 22, 2:50 PM
  • non-minified css in assets

Build is green

Patch application report for D3293 (id=11798)

Rebasing onto bf9e586436...

Current branch diff-target is up to date.
Changes applied before test
commit 9b1c7bf4109135d933aa83eee913d687b7a45911
Author: Daniele Serafini <me@danieleserafini.eu>
Date:   Tue Jun 16 18:24:56 2020 +0200

    dashboard: file visualization per directory path

See https://jenkins.softwareheritage.org/job/DTSCN/job/tests-on-diff/34/ for more details.

vlorentz accepted this revision.Mon, Jun 22, 7:21 PM
This revision is now accepted and ready to land.Mon, Jun 22, 7:21 PM
This revision was automatically updated to reflect the committed changes.