modularity / fedmod

#92 More improvements to summarize-module
Closed by nphilipp. Opened by rdossant.
modularity/ rdossant/fedmod summarize-improve  into  master

Download 92.patch
  • Ignore cached metadata when yaml is passed in the cmdline
  • Glob pattern match filtering
  • Update method documentation
  • Add option to read metadata from a repo URL

Metadata Update from @nphilipp:
- Request assigned

Same old story... :stuck_out_tongue_winking_eye:

For each commit:
- Running flake8 shouldn't flag errors or warnings.
- Unit tests should succeed. E.g. One of the existing module summarizer unit tests verifies the testmodule module which is only available in the repository when it needs to operate on the foo module which is described in spec.v2.yaml.

There's an easy way to run flake8 or the unit tests on all the commits in your branch (assuming you branched off master): running git rebase --exec '...' master which will execute the same command for each commit since master (or your branch off point referenced by hash). Once the command (e.g. flake8 or pytest) fails, it will interrupt the rebase process so things can be fixed & amended, then git rebase --continue.

One other thing I noticed is that you use assert to flag error conditions. There's a problem with that: assert statements won't be run if the interpreter is run with certain flags. Instead of assert, just check the condition and raise click.ClickException(...) with a suitable error description.

Aside from using assert, it would be better just attempting to read the file with libmodulemd because a file ending in .yaml doesn't guarantee that the file actually contains YAML documents.

rebased onto 3ea1489b9c4a7bfdd0933c98bec70eb5bfd18a92

1 new commit added

  • summarizer: replace assert by exception

I didn't find any flake8 messages. Tests were fixed and I replaced the assert by the Exception as asked.

Here's what I mean:

(fedmod) nils@gibraltar:~/src/fedmod/rdossant (rdossant-summarize)> git rebase master --exec 'git log --oneline -1; flake8'
Executing: git log --oneline -1; flake8
3ea1489 (HEAD) summarizer: ignore cached metadata when given a yaml
./_fedmod/modulemd_summarizer.py:28:1: I100 Import statements are in the wrong order. 'from collections import defaultdict' should be before 'import smartcols' and in a different group.
./_fedmod/modulemd_summarizer.py:28:1: I201 Missing newline between import groups. 'from collections import defaultdict' is identified as Stdlib and 'import smartcols' is identified as Third Party.
warning: execution failed: git log --oneline -1; flake8
You can fix the problem, and then run
  git rebase --continue
(fedmod) nils@gibraltar:~/src/fedmod/rdossant ((no branch, rebasing rdossant-summarize))>

And what i mean is:

$: flake8 _fedmod/modulemd_summarizer.py 
$:

So flake8 doesn't give me the same warnings it gives you. I wonder why. Do you use some specific config for flake8?

If we do add a flake8 configuration file (.flake8, tox.ini or whatever), can we switch to a longer line limit? - 80 is distinctly annoying and, in my opinion, decreases the legibility of code by forcing a lot of line folding. A line length limit of 100 is a common value for many projects

@rdossant:

So flake8 doesn't give me the same warnings it gives you. I wonder why. Do you use some specific config for flake8?

I use the plain packaged version from Fedora 29 without any custom configuration on my end. To rule the latter out, I've run it with --config=/dev/null, to the same result:

(fedmod) nils@gibraltar:~/src/fedmod/rdossant (rdossant-summarize)> rpm -q python3-flake8
python3-flake8-3.5.0-6.fc29.noarch
(fedmod) nils@gibraltar:~/src/fedmod/rdossant (rdossant-summarize)> which flake8
/usr/bin/flake8
(fedmod) nils@gibraltar:~/src/fedmod/rdossant (rdossant-summarize)> flake8 --config=/dev/null
./_fedmod/modulemd_summarizer.py:27:1: I100 Import statements are in the wrong order. 'import tempfile' should be before 'from gi.repository import Modulemd' and in a different group.
./_fedmod/modulemd_summarizer.py:28:1: I201 Missing newline between import groups. 'import smartcols' is identified as Third Party and 'import tempfile' is identified as Stdlib.
./_fedmod/modulemd_summarizer.py:29:1: I100 Import statements are in the wrong order. 'from fnmatch import fnmatch' should be before 'import smartcols' and in a different group.
./_fedmod/modulemd_summarizer.py:29:1: I201 Missing newline between import groups. 'from fnmatch import fnmatch' is identified as Stdlib and 'import smartcols' is identified as Third Party.
./_fedmod/modulemd_summarizer.py:30:1: I201 Missing newline between import groups. 'from click import ClickException' is identified as Third Party and 'from fnmatch import fnmatch' is identified as Stdlib.
./_fedmod/modulemd_summarizer.py:31:1: I100 Import statements are in the wrong order. 'from collections import defaultdict' should be before 'from click import ClickException' and in a different group.
./_fedmod/modulemd_summarizer.py:31:1: I201 Missing newline between import groups. 'from collections import defaultdict' is identified as Stdlib and 'from click import ClickException' is identified as Third Party.
./_fedmod/modulemd_summarizer.py:33:1: I101 Imported names are in the wrong order. Should be _fetchrepodata, _repodata

@otaylor, fair enough, now I've found a font which I can read well at 11pt which I need to display two terminals of 100(+2) chars side-by-side (DejaVu Sans Mono): commit d899df1

 
$: rpm -q python3-flake8
python3-flake8-3.5.0-6.fc29.noarch
$: which flake8
/usr/bin/flake8
$: flake8 --config=/dev/null
$:

Meanwhile we've found the culprit to be that I have the flake8-import-order plugin installed. I'll document this so contributions shouldn't be running into this particular snag in the future.

Applied with the discussed changes (import order) in commit b685535.

Pull-Request has been closed by nphilipp
Metadata
nphilipp
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.