Skip to content

feat: ability to download reports as pdf#33

Merged
kpiwko merged 3 commits into
mainfrom
dashboard-link-fixes
Aug 29, 2025
Merged

feat: ability to download reports as pdf#33
kpiwko merged 3 commits into
mainfrom
dashboard-link-fixes

Conversation

@kpiwko

@kpiwko kpiwko commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

This PR does following:

  • updates dashboard links to new URL format and additional teams
  • increases PDF download timeout as logilica now allows that
  • adds option to just download PDFs, do no processing

You can run it for instance with this command, that downloads all reports in yaml configuration and stores them in a reports-july folder that remains after execution:

mkdir reports-july
logilica-cli -v -v weekly-report -t reports-july -O pdf

The other changes are to introduce pre-commit and align local execution with CI execution to make it easier to use.

Comment thread .github/workflows/ci.yaml Outdated
@kpiwko

kpiwko commented Jul 1, 2025

Copy link
Copy Markdown
Contributor Author

Core updates are about adding pdf export option to cli, increasing timeout to 120 seconds and updating the links to the dashboards.

You can run it for instance with this command, that downloads all reports in yaml configuration and stores them in reports-july folder that stays after the execution:

mkdir reports-july
logilica-cli -v -v weekly-report -t reports-july -O pdf

The other changes are 'just' to enable ruff, black and other linting/formatting tools to play nicely together and give you also an option to fix locally.

webbnh

This comment was marked as resolved.

@kpiwko

kpiwko commented Jul 2, 2025

Copy link
Copy Markdown
Contributor Author

This looks like great stuff, but...these changes should have been presented as two (or even three) separate PRs -- one for the infrastructure change and one for the content change (and maybe one for the Dashboard updates) -- these are unrelated to each other, and the infrastructure change is substantial enough that it obscures the more significant change to the content. (And, you should update the PR Description (you can edit it after the fact!) to explain what is going on here (e.g., include the text from your follow-up comment).)

+1. I was about to submit only one line change and yaml updates however somehow I was not able to get all the linters to agree. I'm more than happy to split this into 2 PRs (or two commits in this PR).

Beyond that, I'm requesting that we keep the documentation line limit at 79 characters.

Will do.

And, of course, I have the usual handful of other comments.

Thank you!

@kpiwko kpiwko force-pushed the dashboard-link-fixes branch 3 times, most recently from 3853d2f to c47ab45 Compare July 2, 2025 07:34
@kpiwko

kpiwko commented Jul 2, 2025

Copy link
Copy Markdown
Contributor Author

I have reorganized commits into separate logical pieces (git add -p has been a great help here) and force pushed to this branch. All your comments shoud be resolved now @webbnh.

There are still some formatting changes that black produced - I don't know why, it seems that 88 is the default line length.

webbnh

This comment was marked as resolved.

@kpiwko

kpiwko commented Jul 4, 2025

Copy link
Copy Markdown
Contributor Author

Where do we stand with formatting docstrings to 79 characters instead of 88? I think ruff can do this (see doc-line-too-long (W505) and max-doc-length).

It is already using 79.

ruff can warn on those but not fix. docformatter cannot fix it neither:

docformatter --black --recursive --check --config ./pyproject.toml logilica_cli tests
docformatter --black --recursive --in-place --config ./pyproject.toml logilica_cli tests

Docformatter needs to be set to force-wrap mode. I don't mind setting force wrap and 505 docstring length rule. The other option is to ignore it. What do you prefer?

webbnh
webbnh previously approved these changes Jul 7, 2025

@webbnh webbnh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good, once we settle this last detail:

Where do we stand with formatting docstrings to 79 characters instead of 88? I think ruff can do this (see doc-line-too-long (W505) and max-doc-length).

It is already using 79.

So, the approach is that we will run docformatter and its [tool.docformatter] specifies 79, and we will run ruff and it will ignore docstrings? If so, that works for me.

ruff can warn on those but not fix. docformatter cannot fix it neither:
[...]
Docformatter needs to be set to force-wrap mode.

I'm OK with requiring the developer to fix the warnings manually (docstrings are often fragile, especially if they contain any formatted items, and having to fight with the formatter is really unsatisfying).

I don't mind setting force wrap and 505 docstring length rule. The other option is to ignore it. What do you prefer?

If, as the code stands now, we get warnings on mis-formatted doc strings (including lines casually running over 79 characters) then I propose leaving it as is.

In which case, I think this PR is ready to merge...so long as you remember to revert the change in the reference to python_lint_and_test.yaml in .github/workflows/ci.yaml. 🙂

Comment thread .pre-commit-config.yaml
Comment thread .github/workflows/ci.yaml Outdated
Comment thread logilica_cli/__main__.py Outdated
@kpiwko kpiwko force-pushed the dashboard-link-fixes branch from 4dd4040 to ccdc5c4 Compare August 29, 2025 08:39
@kpiwko

kpiwko commented Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

Validated the fixes with previous force push and now removed temporary linting branch pointer. CI will fail, however that is expected and I'll merge.

@kpiwko kpiwko merged commit 60a7ed4 into main Aug 29, 2025
3 of 5 checks passed
@kpiwko kpiwko deleted the dashboard-link-fixes branch August 29, 2025 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants