Add support for JSON output #5
Conversation
txels
force-pushed
the
main
branch
2 times, most recently
from
c7c018c to
3bdf4b8
Compare
|
I'm on a holiday until 2nd of December but will review after, thanks for your contribution. 🙏 |
src/pypi_changes/_print.py
Outdated
|
|
||
| current_release = pkg.current_release | ||
| current_release_at = current_release.get("upload_time_iso_8601") | ||
| last_release = pkg.last_release or {} | ||
| last_release_at = pkg.last_release_at | ||
|
|
||
| if current_release_at is not None: | ||
| text.append(" ") | ||
| text.append(naturaltime(now - current_release_at), "green") | ||
| if pkg.version != last_release.get("version"): | ||
| text.append(f" remote {last_release.get('version')}", "red") |
There was a problem hiding this comment.
How come we needed to change the rich layer to add the JSON presentation?
There was a problem hiding this comment.
The output hasn't changed to the best of my knowledge. This is a refactor to try and get all of the common "data analysis and aggregation" logic outside of the presentational layer so that it can be used by the various output formats (rich, JSON, and potential future additions).
src/pypi_changes/_print.py
Outdated
| tree.add(text) | ||
| rich_print(tree) | ||
|
|
||
|
|
||
| def release_info(release: dict[str, Any] | None, now: datetime) -> dict[str, Any]: |
There was a problem hiding this comment.
I'd prefer to make the _print a package that has a two modules rich and json. And each file can only contain it's own logic. Can you do that please?
There was a problem hiding this comment.
I've done the split into _print.json and _print.rich. Not sure about what naming conventions you want to follow for these modules.
|
Thanks for the review and the gitignore tip! I'm busy and on a business trip so I don't think I'll tackle this before next week. |
|
@txels do you still intend to finish this? |
Hey, yeah - I was waiting for some feedback from my latest replies (e.g. #5 (comment)) before going forward 😬 as I'd prefer to do all the work in a single sitting. I'd be happy to push this through but it's up to you. I had mixed feelings re: how favorable you were to this PR, TBH. |
|
I'm happy to accept this change we just need to bikeshed on how it would operate. Do you still intend to action on the outstanding points and fix the merge conflicts? |
Good timing, I had been very busy with other stuff and TBH seeing that there were merge conflicts put me off a bit last time I looked. Next week I will have time in the mornings and will be happy to push this forward. I may need to review the previous merged PRs (that have introduced the conflicts) to see what changes were introduced, because as I refactored quite a bit of the original code merging will be tricky. Good that we can rely on great test coverage. |
txels
force-pushed
the
main
branch
2 times, most recently
from
1251336 to
72c89ae
Compare
|
I've rebased and done all the changes you suggested @gaborbernat - tried to separate the commits to make the review easier:
I've also fixed an incorrect mock that made I'm bumping into this issue in CI, where coverage report is failing with a generated file: ...not sure what is the best way to proceed, as I don't understand why this is an issue on my branch in particular. |
txels
force-pushed
the
main
branch
2 times, most recently
from
5a3facb to
4a30b28
Compare
|
Still one open pep8 issue |
|
OK done for today, let me know what you think of the state of this so far and next steps. |
|
@gaborbernat this is now green and conflict free. I don't think I want to spend much more time on this, it's already long gone into a zone of "diminishing returns" for me. |
| @@ -73,8 +73,8 @@ def test_print_alphabetical(capsys: CaptureFixture[str], option_simple: Options, | |||
| assert output[0].startswith("🐍 Distributions within") | |||
| assert output[1] == sys.executable or output[0].endswith(sys.executable) | |||
| assert output[-4:] == [ | |||
| "├── a 1 remote 2 a month ago", | |||
There was a problem hiding this comment.
The original issue was that you were using naturaltime whose type signature accepts datetime|int, which mypy was flagging. Looking into the humanize library, I realised that we should move to use naturaldelta that has the right signature (takes a timedelta). However, that function doesn't include ago in the output.
There was a problem hiding this comment.
TBH looking at the code of humanize the typing is inconsistent, you may consider contributing a patch there.
https://github.com/python-humanize/humanize/blob/main/src/humanize/time.py#L208 <<< naturaltime signature doesn't take a timedelta - however, internally it accepts it https://github.com/python-humanize/humanize/blob/main/src/humanize/time.py#L238
|
Thanks for sticking through it! Now should be released. |
Implements #2
Describe your changes
Adds JSON support via the new CLI flag
--json.Testing performed
Added unit tests, plus tested locally against a few particularly large virtual environments. I've refactored the
print_treeversion to take some logic out of the presentation layer and in the model layer, so that it could be reused for the new JSON output format (and potential future options).Sample output
[ { "name": "charset-normalizer", "version": "2.0.7", "up_to_date": false, "current": { "version": "2.0.7", "date": "2021-10-11T21:28:25.641146+00:00", "since": "a month ago" }, "latest": { "version": "2.0.8", "date": "2021-11-24T19:46:01.258828+00:00", "since": "17 hours ago" } }, { "name": "setuptools", "version": "57.4.0", "up_to_date": false, "current": { "version": "57.4.0", "date": "2021-07-19T01:20:16.478220+00:00", "since": "4 months ago" }, "latest": { "version": "59.2.0", "date": "2021-11-19T01:14:16.274179+00:00", "since": "6 days ago" } }, { "name": "black", "version": "21.11b1", "up_to_date": true, "current": { "version": "21.11b1", "date": "2021-11-18T03:58:02.282075+00:00", "since": "7 days ago" }, "latest": { "version": "21.11b1", "date": "2021-11-18T03:58:02.282075+00:00", "since": "7 days ago" } }, ... ]