Page MenuHomeSoftware Heritage
Differential D5213

swh/scanner : Strip root path from json output
ClosedPublic
Actions

Authored by KShivendu on Mar 7 2021, 2:34 PM.

Details

Reviewers
zack
Group Reviewers
Reviewers
Maniphest Tasks
T2731: scanner: strip the path passed as argument from output
Commits
rDTSCNeb0442443402: CLI : Clean path in scan output
Summary

Fixes T2731

Diff Detail

Repository
rDTSCN Code scanner
Branch
strip-json-output
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19819
Build 30777: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 30776: arc lint + arc unit

Event Timeline

KShivendu created this revision.Mar 7 2021, 2:34 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 7 2021, 2:34 PM
KShivendu retitled this revision from Strip root path from json output to swh/scanner : Strip root path from json output.Mar 7 2021, 2:34 PM
swh-public-ci added a comment.Mar 7 2021, 2:36 PM

Build has FAILED

Patch application report for D5213 (id=18670)

Rebasing onto 433f556301...

Current branch diff-target is up to date.
Changes applied before test
commit c5b36a731047234588fcd2336ff8058129d51948
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Sat Mar 6 20:14:38 2021 +0530

    Strip root path from json output

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

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 7 2021, 2:36 PM
Harbormaster failed remote builds in B19763: Diff 18670!
KShivendu updated this revision to Diff 18671.Mar 7 2021, 2:48 PM

Updating D5213: swh/scanner : Strip root path from json output

swh-public-ci added a comment.Mar 7 2021, 2:50 PM

Build has FAILED

Patch application report for D5213 (id=18671)

Rebasing onto 433f556301...

Current branch diff-target is up to date.
Changes applied before test
commit 2ce1b5d929d631fd5f64cd3d54f759802b6eb97c
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Sat Mar 6 20:14:38 2021 +0530

    Strip root path from json output

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

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 7 2021, 2:50 PM
Harbormaster failed remote builds in B19764: Diff 18671!
KShivendu added a comment.Mar 7 2021, 2:52 PM

All the tests are passing when D5211 is applied. It is expected that the build errors will get fixed once D5211 is merged.

zack added subscribers: DanSeraf, zack.Mar 7 2021, 3:03 PM

Tests are still failing on this, so some changes are needed for sure.

In addition, I'm suggesting above to change the interface of the model so that Path is still used, instead of dumbing down path to strings there.

swh/scanner/model.py
155–160

I understand this is not strictly speaking what was asked in the bug you're trying to fix with this diff, but looking at this I find that it's not correct that to_dict returns paths as strings, rather than keeping them as Path. (This is in turn due to what .attributes() does.)

Unless I'm missing something (cc: @DanSeraf for feedback), the model uses Path objects everywhere, and we should keep doing that for all model users. If someone needs to translate those paths to string, they should do that at the last minute.

Additionally, if we keep using path, what you are doing here could be done much more safely. Instead of using .replace you could (and should) use here something like PurePath.relative_to, which is much safer than string replacement.

swh-public-ci added a comment.Mar 8 2021, 10:06 AM

Build has FAILED

Patch application report for D5213 (id=18671)

Rebasing onto 6c191b6eac...

First, rewinding head to replay your work on top of it...
Applying: Strip root path from json output
Changes applied before test
commit 451baabd2f21aedfb1fbbd45e93e092ca49f63a0
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Sat Mar 6 20:14:38 2021 +0530

    Strip root path from json output

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

vlorentz added a subscriber: vlorentz.Mar 8 2021, 10:09 AM

Tests are still failing, but that's caused by an aiohttp released published last saturday with an invalid version number

DanSeraf added inline comments.Mar 8 2021, 2:48 PM
swh/scanner/model.py
155–160

I agree with the fact that the scanner.model should use Path objects everywhere. This should be changed when we will move all the visualization related functions to another sub-module (T2692). The new sub-module should change the Path objects to string before to output the information.

@KShivendu Regarding this diff, as Stefano stated, i think it's for now safe to extract only the relative path with Path.relative_to.

KShivendu updated this revision to Diff 18711.Mar 10 2021, 7:38 AM

Updating D5213: Use Path.relative_to instead of str.replace

KShivendu marked an inline comment as done.Mar 10 2021, 7:39 AM
swh-public-ci added a comment.Mar 10 2021, 7:39 AM

Build is green

Patch application report for D5213 (id=18711)

Rebasing onto 41c5774031...

Current branch diff-target is up to date.
Changes applied before test
commit ff4d4acfb5c54825ed8d506eaed36be503a7a344
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Sat Mar 6 20:14:38 2021 +0530

    Strip root path from json output

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

Harbormaster completed remote builds in B19806: Diff 18711.Mar 10 2021, 7:39 AM
KShivendu requested review of this revision.Mar 10 2021, 7:39 AM
zack requested changes to this revision.Mar 10 2021, 12:27 PM

Thanks, this fix looks good.

However, it introduces an inconsistency between the json (which now strips the root path) and the text output (which does not strip it).

Can you make the textual output behave the same? (and retitle this diff accordingly)

This revision now requires changes to proceed.Mar 10 2021, 12:27 PM
KShivendu added a comment.Mar 10 2021, 1:47 PM

Hi @zack
In my opinion, the textual output doesn't print anything other than the directory structure so instead of removing the whole root path we can just put the directory name (it looks better).

website-folder
│   js/
│   │   main.js
│   │   table.js
│   .gitignore
│   table.html
│   index.html
│   login.html

VS

│   js/
│   │   main.js
│   │   table.js
│   .gitignore
│   table.html
│   index.html
│   login.html

But the ndjson output must be stripped.

KShivendu updated this revision to Diff 18724.Mar 10 2021, 1:54 PM

Updating D5213: Fix ndjson and textual output

swh-public-ci added a comment.Mar 10 2021, 1:55 PM

Build is green

Patch application report for D5213 (id=18724)

Rebasing onto 41c5774031...

Current branch diff-target is up to date.
Changes applied before test
commit 9154ec2b8fd5347dec09131e190f5fb82b74c3d6
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Sat Mar 6 20:14:38 2021 +0530

    Clean path in scan output
    
    Clean scan outputs for ndjson,json and text format

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

Harbormaster completed remote builds in B19819: Diff 18724.Mar 10 2021, 1:55 PM
zack requested changes to this revision.Mar 10 2021, 4:41 PM
In D5213#132637, @KShivendu wrote:

In my opinion, the textual output doesn't print anything other than the directory structure so instead of removing the whole root path we can just put the directory name (it looks better).
[...]
But the ndjson output must be stripped.

OK, I'm sold, thanks!

But please also add yourself to CONTRIBUTORS as part of this diff.
Then I'll be happy to accept this one.

Also, minor point, we usually use "Closes XXX" in commit messages rather than "Fixes XXX". (They probably both work, but I haven't checked, and consistency is better anyway :-))

This revision now requires changes to proceed.Mar 10 2021, 4:41 PM
KShivendu updated this revision to Diff 18736.Mar 10 2021, 5:29 PM

Updating D5213: swh/scanner : Add Kumar Shivedu to CONTRIBUTORS

swh-public-ci added a comment.Mar 10 2021, 5:31 PM

Build is green

Patch application report for D5213 (id=18736)

Rebasing onto 41c5774031...

Current branch diff-target is up to date.
Changes applied before test
commit 9c9b39892bb36f9c2c03313ffbcc7f41cc2e0a31
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Sat Mar 6 20:14:38 2021 +0530

    CLI : Clean path in scan output
    
    Clean scan outputs for ndjson, json, and text formats

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

Harbormaster completed remote builds in B19831: Diff 18736.Mar 10 2021, 5:31 PM
KShivendu updated this revision to Diff 18737.Mar 10 2021, 5:32 PM

Updating D5213: swh/scanner : Add newline in CONTRIBUTORS file

swh-public-ci added a comment.Mar 10 2021, 5:33 PM

Build is green

Patch application report for D5213 (id=18737)

Rebasing onto 41c5774031...

Current branch diff-target is up to date.
Changes applied before test
commit eb04424434021f39b8869b32a17c11c18872472f
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Sat Mar 6 20:14:38 2021 +0530

    CLI : Clean path in scan output
    
    Clean scan outputs for ndjson, json, and text formats

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

Harbormaster completed remote builds in B19832: Diff 18737.Mar 10 2021, 5:33 PM
zack accepted this revision.Mar 10 2021, 6:17 PM
This revision is now accepted and ready to land.Mar 10 2021, 6:17 PM
Closed by commit rDTSCNeb0442443402: CLI : Clean path in scan output (authored by KShivendu). · Explain WhyMar 10 2021, 6:55 PM
This revision was automatically updated to reflect the committed changes.
KShivendu added a commit: rDTSCNeb0442443402: CLI : Clean path in scan output.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
scanner/
18 lines74%
Loading...
tests/
3 lines100%
Loading...

Diff 18724

View Options

swh/scanner/model.py

Loading...
View Options

swh/scanner/tests/test_model.py

Loading...
About · Developer information · Legal