Details

Reviewers

vlorentz
ardumont

Group Reviewers

Reviewers

Commits

rDTSCN1541e9f44a9e: default url
rDTSCNc0dbe0b1907b: cli: scanner group with scan option, updated description
rDTSCNdd9004026c04: color only when stdout is connected to terminal, removed logger
rDTSCNed5a359e2ef4: added: model, logger and exceptions
rDTSCN40442b4d6bf8: scanner: asynchronous operation, type annotation
rDTSCNf39b3d2c3374: test requirements
rDTSCNb09cf5b29168: code scanner prototype

Summary

First prototype of the code scanner

Usage example

swh scanner scan "/path/to/scan"

sample output (using swh-py-template as a source path):

swh/
debian/
.git/
│   refs/
│   │   heads/
│   │   │   master
│   │   remotes/
│   │   tags/
│   hooks/
│   │   pre-receive.sample
│   │   pre-push.sample
│   │   update.sample
│   │   pre-rebase.sample
│   │   fsmonitor-watchman.sample
│   │   post-update.sample
│   │   pre-commit
│   │   pre-commit.sample
│   │   prepare-commit-msg.sample
│   │   commit-msg.sample
│   │   pre-applypatch.sample
│   │   applypatch-msg.sample
│   objects/
│   │   pack/
│   │   │   pack-750cdb447ff562005695fc225fc7f81ba936b697.pack
│   │   │   pack-750cdb447ff562005695fc225fc7f81ba936b697.idx
│   │   info/
│   info/
│   logs/
│   │   refs/
│   │   │   heads/
│   │   │   │   master
│   │   │   remotes/
│   │   │   │   origin/
│   │   │   │   │   HEAD
│   │   HEAD
│   branches/
│   description
│   ORIG_HEAD
│   HEAD
│   config
│   FETCH_HEAD
│   packed-refs
│   index
docs/
requirements-swh.txt
README.md
AUTHORS
requirements.txt
MANIFEST.in
pytest.ini
mypy.ini
LICENSE
CONTRIBUTORS
requirements-test.txt
.gitignore
Makefile
tox.ini
setup.py
CODE_OF_CONDUCT.md
.pre-commit-config.yaml

The output has colors indicating if a directory or a file is discovered:
If a directory is blue means that all the contents inside are found, otherwise it's red.
For each red directory, the file is green if it's found otherwise it's red.

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.Feb 12 2020, 12:13 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 12 2020, 12:13 PM

Harbormaster completed remote builds in B10560: Diff 9496.Feb 12 2020, 12:13 PM

I'm missing context on what this code scanner does, so I'm only commenting on documentation for now.

New code should use type annotations, instead of having types in the docstring.

swh/foo/py.typed
1	re-add this file
swh/scanner/cli.py
28	Should have a docstring explaining what it does. And if the whole package contains "scanner tools", then the command should have a more specific name telling what it does
swh/scanner/scanner.py
19–29	Missing description.
60–70	Missing empty lines (see https://sphinxcontrib-napoleon.readthedocs.io/en/latest/ )
79–87	same

This revision now requires changes to proceed.Feb 12 2020, 12:27 PM

The cli supports only one operation: swh scanner [options] path, where path is the root path we want to scan.
The output now is a tree like structure with a green/blue text if the path is found, red otherwise.

Harbormaster completed remote builds in B10762: Diff 9696.Feb 26 2020, 4:38 PM

Looks good.

Mostly nitpicks on docstrings as well:

I'd make the color output optional, that'd make for something more easily parsable.
Can we have a sample output of a run in the diff description, that'd be nice ;)
The logger module seems overkill as i see it used only once though

Cheers,

swh/scanner/exceptions.py
1 ↗	(On Diff #9696)	Please add the headers ;)
swh/scanner/model.py
55 ↗	(On Diff #9696)	use your colorize function.
swh/scanner/scanner.py
31	what about at least the `url` description?
72	As per a recent discussion, you can replace this by `Iterator[Tuple[PosixPath, str]]`. @vlorentz ^ tell me if i misremember ;)
125	fills
version.txt
1 ↗	(On Diff #9696)	Add this file to .gitignore and remove it from the git index ;)

In D2657#64937, @ardumont wrote:

I'd make the color output optional, that'd make for something more easily parsable.

Ack. A good default behavior on this is to colorize output when stdout is connected to a terminal and not do it when it is connected to a pipe. That's what most git commands do and it's a very good default.

It's still missing a description of what it does (ie. check that files exist in swh), in the CLI help and/or README.

and I still think you should use a subcommand, in case you want to add new features in the future without breaking scripts that depend on this one.

swh/scanner/cli.py
29–34	Why not a base URL instead of host+port?

This revision now requires changes to proceed.Feb 27 2020, 3:23 PM

In D2657#64937, @ardumont wrote:

The logger module seems overkill as i see it used only once though

Yes, i see. I manly use it to benchmark different algorithms so i can remove it.

In D2657#65016, @vlorentz wrote:

It's still missing a description of what it does (ie. check that files exist in swh), in the CLI help and/or README.

and I still think you should use a subcommand, in case you want to add new features in the future without breaking scripts that depend on this one.

I will update the description, anyway the only supported command by the module right now is the scan operation so i could add a subcommand when a new feature is needed .

swh/scanner/cli.py
29–34	Yeah, it should be fine with the url only

zack added inline comments.Feb 27 2020, 4:28 PM

swh/scanner/cli.py
29–34	@DanSeraf, I'm already using a `api_url` argument in the (WIP…) API client for the webapp (D2461), let's standardize on that as a function parameter (`api_url`) and as a command line option (`--api-url`), both defaulting to `https://archive.softwareheritage.org/api/1`.

cli: scanner group with scan option, updated description
color only when stdout is connected to terminal

Harbormaster completed remote builds in B10799: Diff 9732.Feb 28 2020, 1:24 PM